Skip to content

Cover the EINTR retry in natsSock_WaitReady with two signal arms - #10

Merged
alexey-milovidov merged 1 commit into
ClickHouse:ClickHouse/v3.9.2from
groeneai:test-waitready-eintr-signal-storm
Sep 21, 2026
Merged

alexey-milovidov merged 1 commit into
ClickHouse:ClickHouse/v3.9.2from
groeneai:test-waitready-eintr-signal-storm

Conversation

@groeneai

Copy link
Copy Markdown

natsSock_WaitReady retries poll() on EINTR since b88704df, but test_natsWaitReady only ever waits on a quiet thread, so no arm in the suite reaches that retry. This adds two arms that deliver a signal to the waiting thread. No library code changes.

Asked for in ClickHouse/ClickHouse#120202 (comment).

Both arms are #ifndef _WIN32:

  • no deadline, signals delivered: the mock server makes the socket readable after ~500 ms, so the wait has to return NATS_OK then, not on the first signal.
  • deadline timeout, signals delivered: a 50 ms deadline over the same storm still has to end in NATS_TIMEOUT within 40 to 100 ms.

Both also assert that at least one signal was delivered, so neither can pass by not being about signals.

Measured on this branch (Linux, -DBUILD_TESTING=ON -DNATS_BUILD_STREAMING=OFF, testsuite natsWaitReady):

library first arm second arm runs
b88704df, this base PASSED PASSED 3/3
cb4edba, poll() unretried FAILED: Error: 3 - IO Error - (unix/sock.c:56): poll error: 4 not reached 3/3
b88704df with the deadline recompute hoisted out of the retry loop PASSED FAILED 2/2

Row 2 is the failure ClickHouse CI reports byte for byte (Cannot connect to Nats last error: (unix/sock.c:56): poll error: 4). Row 3 is why the second arm is not redundant: a retry that re-arms the full deadline instead of what is left of it waits until the storm stops rather than ending on its own 50 ms deadline.

SIGALRM comes from pthread_kill() on a helper thread rather than from setitimer(), because ITIMER_REAL signals the process: the kernel then picks any thread that does not block it, which also cuts short the mock server thread's nats_Sleep(). Targeted delivery keeps the bounds deterministic. The handler stays installed after the storm on purpose, since restoring SIG_DFL would let a signal still in flight terminate the process.

test_natsWaitReady is already in test/list_test.txt, so ctest -R natsWaitReady picks the arms up with no registration change.

#8 carried this test together with its own version of the retry. The retry landed as b88704df instead, so I closed that one; this is the test alone, on top of it.

test_natsWaitReady exercised the wait on a quiet thread only, so the poll()
that b88704d made restartable had no arm that a signal reaches. Two arms
deliver SIGALRM to the waiting thread about every millisecond, one over a wait
with no deadline and one over a wait that must still time out on its own
deadline, and both assert that at least one signal was delivered.

Without the retry the first arm returns NATS_IO_ERROR (poll error: 4) in under
a millisecond instead of NATS_OK after ~500ms, and the second returns the same
error instead of NATS_TIMEOUT. A retry that re-arms the full deadline instead
of what is left of it passes the first arm and fails the second.

SIGALRM is delivered with pthread_kill() from a helper thread rather than by
setitimer(): ITIMER_REAL signals the process, so the kernel picks any thread
that does not block it, which would also cut short the mock server thread's
own nats_Sleep() and make the arms racy.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@rschu1ze

Copy link
Copy Markdown
Member

Upstreamed here: nats-io#1030

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants