Repository navigation
libuv adapter: do not let an attach overtake a queued poll removal - #5
Conversation
The adapter defers poll mutation to the event loop thread through a FIFO, but natsLibuv_Attach did not follow that discipline in two ways. First, it wrote nle->socket and nle->events directly. On a reconnect that runs on the nats-io reconnect thread (_doReconnect -> _processConnInit -> evCbs.attach), those writes race the event loop thread's write in uvPollUpdate's close branch. ThreadSanitizer reports it as a data race on a 4-byte field of natsLibuvEvents. Second, it scheduled only when it was called off the loop thread, while Read/Write/Detach also schedule when events are already queued. A loop-thread attach could therefore run ahead of removals already in the list. Both let an attach install a new socket before an older, already queued READ removal is drained. _evStopPolling only requests the removals and then _processOpError starts the reconnect thread, so when the loop thread finally drains that removal, nle->events reaches 0 and uvPollUpdate takes the close branch on the socket the attach just installed: natsConnection_ProcessCloseEvent closes a live, freshly connected fd and sets nle->socket to invalid. The reconnect still reports success (nc->el.attached = true) but the connection never polls again. Carry the socket in the ATTACH event and assign nle->socket and nle->events inside uvAsyncAttach, so both fields are written only by the loop thread, and give the attach the same "or events are queued" guard as the other three callbacks. Ordering is then enforced by the queue instead of by timing, and the race disappears because the fields are no longer shared. No new lock is introduced; nle->lock still guards only head/tail. Reproduced with a standalone harness that links this header against libuv under ThreadSanitizer and drives the interleaving deterministically: before, 25/25 runs close the freshly attached fd and 23/25 concurrent runs report the data race in natsLibuv_Attach; after, 0 in both.
Internal second-model review (2 findings, both resolved without code change)Before opening this PR I ran an independent cold review plus a second-model review of the diff, then adjudicated every finding against the recorded evidence. Both findings were about verification and documentation rather than the change itself; neither produced a code change.
💡 One note on process, since it affects how much weight to give the gate here. The deliverable is a commit inside a vendored submodule, so my first gate run reviewed the superproject and correctly reported an empty diff, which no re-run would have changed. I re-ran it against a worktree of the submodule at this commit and confirmed the scope was full rather than narrowed: the reviewed change map was "1 file changed, 60 insertions(+), 19 deletions(-)", byte-identical to |
Pre-PR validation gate (click to expand)
Session id: cron:clickhouse-impl-slot-6:20260818-044502 |
2 participants
natsLibuv_Attachcan install a new socket before an already queued READ removal is drained, and that removal then closes the new socket.The adapter defers poll mutation to the event loop thread through the
nle->head/tailFIFO, but attach did not follow that discipline: it wrotenle->socket/nle->eventsinline (on a reconnect that runs on the nats-io reconnect thread,_doReconnect->_processConnInit->evCbs.attach, so those writes raceuvPollUpdate's close branch on the loop thread), and it scheduled onlyif (sched)where_Read/_Write/_Detachusesched || (nle->head != NULL), so a loop-thread attach could jump ahead of queued removals with no second thread involved.Both reach the same outcome.
_evStopPollingonly requests removal of write then read, and_processOpErrorthen starts the reconnect thread. When the loop thread finally drains that older READ removal,nle->eventsreaches 0,uvPollUpdatetakes the close branch, andnatsConnection_ProcessCloseEventcloses the freshly connected fd. The reconnect still reports success (nc->el.attached = true) while the connection never polls again, so a subscriber goes permanently silent after a broker restart.Fix: carry the socket in the
NATS_LIBUV_ATTACHevent and assignnle->socket/nle->eventsinsideuvAsyncAttach, so both are written only by the loop thread; and give attach the same "or events are queued" guard as the other three callbacks. Ordering is then enforced by the queue rather than by timing, and the race disappears because the fields are no longer shared. No new lock. First attach is unaffected: a freshlycalloc'dnlehashead == NULL.Found by ThreadSanitizer in ClickHouse CI, which vendors this adapter (1 hit in 1455 tests, alongside the NATS server becoming unreachable during a broker-restart test):
CI report
That window is narrow, so I verified with a standalone harness linking this header against libuv under ThreadSanitizer, injecting the interleaving rather than waiting for it. Before, 25/25 deterministic runs close the new fd and 23/25 concurrent runs report the race; after, 0 in both, with the close path still exercised.
Validation detail, test-suite gap, and upstream context
The harness oracle is the outcome rather than the sanitizer warning: it counts closes of an fd installed by an attach after the drained removal was queued, so it cannot pass by simply losing coverage (a run that never reaches the close branch is reported as a broken probe). A separate probe demonstrates the loop-thread queue-jump with no second thread involved: it fails before and passes after.
I did not add a test to
test/test.c: libuv is not part of this repo's build or CI (NATS_BUILD_LIBUV_EXAMPLEdefaults OFF and needs a caller-suppliedLIBUV_DIR), and the existing event-loop test registers mock callbacks rather thannatsLibuv_*. That is also why this went unnoticed. Happy to add one if you want libuv wired into the test build.Both defects are verbatim in current upstream
nats-io/nats.c; I am filing an issue there too. Upstream nats-io#814 (closed) produced the split-close design this bug lives in; nats-io#1005/nats-io#1006 touch the same function but are a different defect, deliberately not folded in.adapters/libevent.hhas a structurally similar attach but keeps no socket field (it storesstruct event *and reads the fd back viaevent_get_fd), so it is unaffected.