Cover the EINTR retry in natsSock_WaitReady with two signal arms - #10
Merged
alexey-milovidov merged 1 commit intoSep 21, 2026
Conversation
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>
This was referenced Sep 21, 2026
alexey-milovidov
approved these changes
Sep 21, 2026
Member
|
Upstreamed here: nats-io#1030 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
natsSock_WaitReadyretriespoll()onEINTRsinceb88704df, buttest_natsWaitReadyonly 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:NATS_OKthen, not on the first signal.NATS_TIMEOUTwithin 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):b88704df, this basecb4edba,poll()unretriedError: 3 - IO Error - (unix/sock.c:56): poll error: 4b88704dfwith the deadline recompute hoisted out of the retry loopRow 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.SIGALRMcomes frompthread_kill()on a helper thread rather than fromsetitimer(), becauseITIMER_REALsignals the process: the kernel then picks any thread that does not block it, which also cuts short the mock server thread'snats_Sleep(). Targeted delivery keeps the bounds deterministic. The handler stays installed after the storm on purpose, since restoringSIG_DFLwould let a signal still in flight terminate the process.test_natsWaitReadyis already intest/list_test.txt, soctest -R natsWaitReadypicks the arms up with no registration change.#8 carried this test together with its own version of the retry. The retry landed as
b88704dfinstead, so I closed that one; this is the test alone, on top of it.