Skip to content

EventEmitter: offAndWait()/removeAllListenersAndWait() for the destroy-vs-in-flight-handler race#93

Merged
ptesavol merged 1 commit into
mainfrom
claude/distracted-bhaskara-cf2e2a
Jul 17, 2026
Merged

EventEmitter: offAndWait()/removeAllListenersAndWait() for the destroy-vs-in-flight-handler race#93
ptesavol merged 1 commit into
mainfrom
claude/distracted-bhaskara-cf2e2a

Conversation

@ptesavol

Copy link
Copy Markdown
Collaborator

Problem

An owner that removes a listener and immediately frees the state the listener captured (the ListeningRpcCommunicator::destroy() pattern behind the phase-C6 SIGSEGVs) can race a handler invocation still running on another thread — a use-after-free. The phase-C6 attempt made off() itself wait for in-flight invocations and was reverted before merge: it hung Layer1ScaleTest.MultipleLayer1Dht and ContentDeliveryLayerNodeLayer1Test.HappyPathSixtyFourNodes.

Root cause of the hang (runtime-proven with thread samples of the hung tests)

Blocking off() deadlocks in two distinct shapes, so it cannot be the default:

  1. Handler↔handler cycle — one worker ran a connection's Data handler (handshake success → stopHandshaker()off<Disconnected>) while another ran the same connection's Disconnected handler (close tie-break → off<Data>); each waited forever for the other's invocation to end. One of the two workers was a Simulator association-executor delivery task, so its inFlightOperations decrement never ran — that is exactly why the hang surfaced as Simulator::stop()'s drain never completing.
  2. Caller-held-lock ABBAConnectionManager::acceptNewConnection holds the Endpoint state mutex while detachFromPendingConnection removes the Connected listener; the in-flight Connected handler blocks on that same mutex.

Design

  • off()/removeAllListeners() keep their historical non-blocking semantics.
  • New offAndWait()/removeAllListenersAndWait() remove the listener(s), cancel not-yet-started invocations of an executing emit loop, and wait until no invocation runs on another thread.
  • The wait is skipped when the caller is itself inside any handler invocation, tracked by a thread_local invocation depth bumped around every handler call. A blocked waiter therefore never runs a handler itself, so no cycle of handlers waiting on each other can form (shape 1). Waiting callers must not hold locks the event's handlers take (shape 2) — documented on the methods.
  • ListeningRpcCommunicator::destroy() now uses the waiting variants.
  • Simulator hardening found during the investigation: folly::SerialExecutor swallows task exceptions, so a throwing delivery call-out would silently skip the inFlightOperations decrement and park stop()'s drain forever. The decrement now runs on all paths (exception logged). No such exception was observed at runtime; this closes the hazard.

Tests

  • OffWaitsForInFlightHandlerBeforeOwnerFreesState / RemoveAllListenersWaitsForInFlightHandler — the destroy-vs-in-flight-handler regression tests; they fail under non-waiting semantics.
  • HandlersRemovingEachOtherAcrossThreadsDoNotDeadlock — replica of deadlock shape 1; completes via the depth guard.
  • Scale verification: reintroducing blocking off() deadlocked 8 s into MultipleLayer1Dht (two workers parked forever, drain starved); with this design MultipleLayer1Dht and the 64-node test passed 8/8 runs (instrumented + clean builds) with zero blocked-wait diagnostics. Full emitter suite 22/22; streamr-eventemitter and streamr-dht lint clean.

🤖 Generated with Claude Code

@github-actions github-actions Bot added the dht label Jul 13, 2026
…y-vs-in-flight-handler race

An owner that removes a listener and immediately frees the state the
listener captured (the ListeningRpcCommunicator::destroy() pattern from
the phase-C6 crashes) could race a handler invocation still running on
another thread — a use-after-free. The phase-C6 attempt to close this
made off() itself wait for in-flight invocations and had to be reverted:
it hung Layer1ScaleTest.MultipleLayer1Dht and the 64-node
ContentDeliveryLayerNode test.

Reproducing the hang with thread samples showed two deadlock shapes, so
blocking cannot be the default off() semantics:

1. Handler-handler cycle: one thread ran a connection's Data handler
   (handshake success -> stopHandshaker -> off<Disconnected>) while
   another ran the same connection's Disconnected handler (close
   tie-break -> off<Data>); each waited forever for the other. One of
   the two threads was a Simulator association-executor delivery task,
   which is why the hang surfaced as Simulator::stop()'s
   inFlightOperations drain never reaching zero.
2. Caller-held-lock ABBA: ConnectionManager::acceptNewConnection holds
   the Endpoint state mutex when detachFromPendingConnection removes the
   Connected listener, while the in-flight Connected handler blocks on
   that same mutex.

The landed design keeps off()/removeAllListeners() non-blocking (their
historical semantics) and adds explicit waiting variants:

- offAndWait()/removeAllListenersAndWait() remove the listener(s),
  cancel not-yet-started invocations of an executing emit loop, and wait
  until no invocation is running on another thread.
- The wait is skipped when the caller is itself inside any handler
  invocation, tracked by a thread-local invocation depth bumped around
  every handler call: every blocked waiter then runs no handler itself,
  so no waiter can be part of a cycle of handlers waiting for each other
  (shape 1). Callers must not hold locks the event's handlers take
  (shape 2) — documented on the methods.
- ListeningRpcCommunicator::destroy() uses the waiting variants.

Also hardened the Simulator's delivery lambda: folly::SerialExecutor
swallows task exceptions, so an exception thrown by a delivery call-out
would silently skip the inFlightOperations decrement and park stop()'s
drain forever. The decrement now runs on all paths and the exception is
logged. (No such exception was observed at runtime; this closes the
hazard found while investigating.)

Regression tests: offAndWait/removeAllListenersAndWait complete only
after the in-flight handler finishes (fails on the non-waiting
semantics), and the shape-1 deadlock replica (two handlers removing each
other across threads) completes without deadlock.

Verification: the reintroduced blocking-off() build deadlocked 8 seconds
into Layer1ScaleTest.MultipleLayer1Dht; with this design the dht scale
test and the 64-node test passed 8/8 runs (instrumented and clean) with
zero blocked-wait log lines.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ptesavol
ptesavol force-pushed the claude/distracted-bhaskara-cf2e2a branch from 09c3e39 to ad7ffc4 Compare July 13, 2026 22:18
@ptesavol

Copy link
Copy Markdown
Collaborator Author

CI failure diagnosis: pre-existing hang, not this change — rebased onto main

The macOS-leg failure (WebrtcFullNodeNetworkTest.HappyPath, 1200 s ctest timeout) is the pre-existing joinScope teardown-drain hang, not the emitter change. Evidence:

  • The CI log shows SetUp crawling ~20 min, throwing "Timed out" (the connectivity wait), and TearDown then hanging silently until the ctest kill — the same signature PR Phase D1: streamrNode* C API — the full network node in the shared library #90's investigation hit on ubuntu-latest before this branch existed.
  • Reproduced locally by pinning SharedExecutors to the CI runner's pool sizes (worker=4, background=2): 6/16 runs hang on the pre-rebase binary and 1/8 on a rebased binary. In all seven captured thread samples the main thread is in NetworkStack::stop() → GuardedAsyncScope::close() → Baton::wait and there are zero offAndWait frames — the new wait paths are not involved.
  • The full drain fix (cancel-before-drain + closeAsync) is still in flight from the PR Phase D1: streamrNode* C API — the full network node in the shared library #90 follow-up work; until it lands this test can still flake on starved runners at a low rate.

Rebased onto current main (post-#90) and re-verified locally: emitter suite 22/22, Layer1ScaleTest.MultipleLayer1Dht pass, 64-node pass, WebrtcFullNodeNetworkTest.HappyPath pass.

@ptesavol
ptesavol merged commit 877a098 into main Jul 17, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant