fix(cluster): synchronize shared-listener start-once to stop worker-listen race#1240
Merged
Conversation
…isten race
Root-cause a flaky 60s CI timeout in the cluster port-sharing tests
(RoundRobin_DistributesConnectionsAcrossWorkers, Fork_WorkersShareNetPort,
Fork_WorkersShareHttpPort), Compiled mode. It was not test flakiness but a real
data race in SharedListenerRegistry.
SharedTcpListener.AddWorker / SharedHttpListener.AddWorker started the underlying
listener with an unsynchronized check-then-act:
if (!_started) { _started = true; _listener.Start(); StartAccepting(); }
Under SCHED_RR both cluster workers call server.listen() on the same port from
their own interpreter threads, and ConcurrentDictionary.GetOrAdd hands both the
SAME SharedTcpListener. When the two AddWorker calls overlap, both pass the
`!_started` check and both call _listener.Start(); the second Bind on the same
socket throws. That exception propagates out of ListenAsClusterWorker before it
fires the `listening` callback / process.send('ready'), so the primary's
readyCount never reaches 2, the connections are never made, and the module hangs
to the 60s timeout. Multi-core + CI load widens the window, so it surfaced
intermittently and as a timeout (not an assertion failure).
Wrap the start-once transition in a lock (double-checked, publishing `_started`
only after Start() succeeds so a genuinely failed first Start can be retried) in
both shared listeners, and guard Stop() with the same lock.
Reproduced locally on the pre-fix build: Fork_WorkersShareNetPort(Compiled) hung
61s on the first loop iteration. Post-fix: 40/40 iterations of the three
port-sharing tests pass with no hang; full ClusterModule/Net/Http suite 130/0.
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.
Summary
Fixes the intermittent 60s CI timeout in the cluster port-sharing tests (
RoundRobin_DistributesConnectionsAcrossWorkers,Fork_WorkersShareNetPort,Fork_WorkersShareHttpPort, Compiled mode). This is a real data race, not test flakiness — pre-existing onmain.It surfaced on the CI run for #1239 (inner-function fix), was root-caused there, but the fix landed on the branch after #1239 had already merged — so it never reached
main. This PR lands it.Root cause
SharedTcpListener.AddWorker/SharedHttpListener.AddWorker(Runtime/Types/SharedListenerRegistry.cs) started the underlying listener with an unsynchronized check-then-act:Under
SCHED_RR, both cluster workers callserver.listen()on the same port from their own interpreter threads, andConcurrentDictionary.GetOrAddhands both the sameSharedTcpListener. When the twoAddWorkercalls overlap:_started == false, both set ittrue, and both call_listener.Start();Bindon the same socket throws;SharpTSNetServer.ListenAsClusterWorkerbefore it fires thelisteningcallback /process.send('ready');readyCountnever reaches 2, the connections are never made, and the module hangs to the 60s timeout.Multi-core + CI load widens the race window, so it appeared intermittently, Compiled-mode only, and as a timeout (not an assertion failure).
Fix
Wrap the start-once transition in a lock — double-checked, publishing
_startedonly afterStart()succeeds (so a genuinely failed firstStartcan still be retried) — in bothSharedTcpListenerandSharedHttpListener, and guardStop()with the same lock.Verification
Fork_WorkersShareNetPort(mode: Compiled)hung 61s on the first iteration; the next passed in 7s.ClusterModule/Net/Httpsuite: 130 / 0.