Repository navigation
Destroy the protocol parser on disconnect when an event loop is attached (v3.13.0) - #15
Open
alexey-milovidov wants to merge 2 commits into
Open
alexey-milovidov wants to merge 2 commits into
alexey-milovidov wants to merge 2 commits into
Conversation
Since upstream b89b44d ("Libuv may crash if connection destroyed while consuming"), `natsLibuv_Read` calls `uv_poll_stop` on removal before it decides whether to schedule the event. `natsConnection_Close` reaches it from the thread that closes the connection, so `uv_poll_stop` modifies the poll watcher of a loop running in another thread. Libuv is not thread-safe: ThreadSanitizer reports a data race between `uv__io_stop` and `uv__io_poll`, and the corrupted watcher state can hang the close. Stop polling immediately only from the event loop thread, and only while the poll handle still exists; from other threads the scheduled removal stops the poll on the loop thread. The connection stays retained until the adapter detaches, so a read event delivered in between is harmless. Found by ClickHouse CI: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=119507&sha=acb8e8bda856c88eda8525e5abbeec03074b0ce4&name_0=PR&name_1=Integration%20tests%20%28amd_tsan%2C%20targeted%29 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Backport of nats-io#1008 by @ arnaudhe, which fixes nats-io#1007. Both are still open upstream, where the only discussion is how to test the change, so this carries the patch onto the branch ClickHouse pins and adds the regression test upstream does not have. The parser holds half-decoded bytes that belong to exactly one socket, so it must be discarded before any byte from the next socket is parsed. The library honours that on the blocking-IO path and violates it on the external event-loop path. `natsParser_Destroy` has two call sites in `src/conn.c`: `_freeConn`, and the exit of `_readLoop`, which also NULLs `nc->ps`. `_readLoop` is the blocking-IO reader and every reconnect creates a new one, so there the parser is per socket. When `nc->opts->evLoop` is set, `_processConnInit` attaches the adapter instead, reads arrive through `natsConnection_ProcessReadEvent`, and that function creates the parser lazily and never destroys it. The disconnect notification point for that path is `_processOpError`, which stops polling and leaves `nc->ps` alone. A drop while the parser is inside `MSG_PAYLOAD` therefore survives into the reconnect, and the next bytes read on the new socket are appended to the stale pending payload. Measured on the unpatched tree with the new test: a header announcing 12 payload bytes followed by 4 of them, then a close, then `MSG foo 1 5\r\nhello\r\n` on the reconnected socket, delivers `natsConn_processMsg(buf="AAAAMSG foo ", bufLen=12)` to the subscriber. The stale parser takes the first 8 bytes of the new protocol line as the payload it was still owed, `MSG_END` then skips to the next `\n`, and the leftover `hello` is not a valid op, so the connection is torn down with `NATS_PROTOCOL_ERROR` right after handing out the wrong message. The handshake itself does not pass through the parser and cannot clear it: `_processExpectedInfo` reads the greeting through `_readProto`, which loops `natsSock_Read` one byte at a time, and `_sendConnect` reads the PONG the same way. The flag is deferred rather than a destroy at the disconnect because both direct alternatives are unavailable. `natsConnection_ProcessCloseEvent` takes `(natsSock *socket)` and has no connection pointer, and the adapters call it as `natsConnection_ProcessCloseEvent(&(nle->socket))`, so widening it is an ABI break for every adapter in tree and out. Destroying `nc->ps` inside `_processOpError` would be a use-after-free: `natsConnection_ProcessReadEvent` unlocks before calling `natsParser_Parse`, and `_processOpError` is entered from the ping timer and the write path on other threads. Setting the flag under `natsConn_Lock` and consuming it at the top of the next `ProcessReadEvent` keeps the destroy on the event-loop thread, where the previous socket's `natsParser_Parse` has already returned. Two comments are reworded from upstream's, and nothing else in the code differs: the `natsp.h` field's trailing comment and the one in `_processOpError` both say "external event loop" instead of naming libuv, because the guard is `nc->el.attached`, which is any adapter, and the test below drives the suite's generic event loop. The new `EventLoopParserResetOnDisconnect` needs no broker binary and no libuv. It drives the suite's generic external event loop, whose thread calls the patched `natsConnection_ProcessReadEvent`, against a two-accept `_startMockupServer` thread that serves the initial connection and the reconnect on one listener. Reading the resent `SUB` is what orders the post-reconnect send after the handshake, so the test does not sleep. It is a new registered case rather than an extension of `test_EventLoop` or `test_EventLoopRetryOnFailedConnect` because both of those drive a real `nats-server` through `_startServer`, and a real server never announces 12 payload bytes and then sends 4, so the defect is unreachable from either. The three checks do not return on failure, so the event loop thread is always stopped by the teardown before the library is closed. Validated with clang-22, Debug, `BUILD_TESTING=ON`, `NATS_BUILD_STREAMING=OFF`, `NATS_BUILD_LIB_STATIC=ON`, out of tree. With the two `src/` hunks reverted and the test present the case fails on 5 of 5 runs of this tree, always at the content check, with the payload above; with them applied it passes on 7 of 7. Two earlier forms of the test, before its teardown and comments were cleaned up, measured the same direction on their own binaries: 13 base failures, 0 base passes, 0 fix failures. The 114 tests of this suite that need no broker binary go 113 of 114 on the reverted tree, the single failure being this new case, and 114 of 114 with the fix, so nothing else moves. The two source files also build clean in ClickHouse's own configuration, which is where this branch is consumed. Reported on: ClickHouse/ClickHouse#76867 (comment) CI report: https://s3.amazonaws.com/clickhouse-test-reports/praktika.html?PR=76867&sha=dfc5ef24014997273075680988a2e6a1712b21b3&name_0=PR&name_1=Integration%20tests%20(amd_asan_ubsan,%20db%20disk,%20old%20analyzer,%204%2F8) Related: ClickHouse/ClickHouse#116417 (comment) Related: nats-io#1007 Related: nats-io#1008 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Cherry-pick of 19edbc4 (#9, backport of nats-io#1008) from
ClickHouse/v3.9.2onto the v3.13.0 line, so thenats-iobump does not regress it.Stacked on #14 (
ClickHouse/v3.13.0-libuv-read-loop-thread); merge that first.Related: ClickHouse/ClickHouse#119507
Related: #9
🤖 Generated with Claude Code