fix(bridge): enforce TaskCompletionSource handshake gate to prevent connection race#3110
fix(bridge): enforce TaskCompletionSource handshake gate to prevent connection race#3110HarshalPatel1972 wants to merge 5 commits into
Conversation
|
Will twill try to look tomorrow before the next 3.0.x release Note that since this targets "main" which is now V3, there is no more pipe-reader, but that doesn't mean the async goes away. However, there is now technically support for running the read loop on dedicated sync IO threads - I have not exposed this on the public API yet, however. |
HarshalPatel1972
force-pushed
the
fix/connection-state-race
branch
from
b0ff60e to
81c9788
Compare
| { | ||
| var options = new ConfigurationOptions | ||
| { | ||
| EndPoints = { { TestConfig.Current.MasterServer, TestConfig.Current.MasterPort } }, |
There was a problem hiding this comment.
I'm kinda confused here... this hasn't existed for 4+ years... was this PR based on a really old fork?
| { | ||
| // Simulate severe thread pool exhaustion using ThreadPool.SetMinThreads(1, 1); | ||
| ThreadPool.GetMinThreads(out int workerThreads, out int completionPortThreads); | ||
| ThreadPool.SetMinThreads(1, 1); |
There was a problem hiding this comment.
this isn't a great idea in a CI setup; I understand the intent, but... this might be something we simply can't do in this context due to parallelism concerns, plus the impact on the overall runtime
|
The CI seems to hate this; I suspect it is causing a sync-over-async that is blowing up the thread-pool; if it doesn't turn out to be a false positive, I might take the premise and start from scratch in terms of fixes. |
|
Additionally: given that this PR seems to have targeted 4+ year old code, I'm suspicious that it may be based on out-of-date logic. Either way: I don't think we can take the PR as is. I'm going to close the PR, but I'd love to discuss the topic in an issue, where the first question will be "With what library version are you seeing this?" |
@mgravell Ah, my apologies for the confusion! I accidentally targeted the PR against main (V3), but my branch was actually based off the 2.13.17 release tag where the PipeReader architecture still exists. That explains why the code looked 4+ years out of date to you! We were seeing this race condition on 2.13.17 under heavy load when the thread pool gets temporarily starved. The test case with ThreadPool.SetMinThreads(1, 1) was definitely a sledgehammer approach just to reliably reproduce the starvation locally—I completely understand why CI hated it and why it can't be merged as-is. Since main has moved on to V3 and the internal async architecture has changed so much, I agree this specific patch doesn't fit anymore. If you think this race condition might still exist in the new V3 architecture, I'd love to help test any new approaches or dedicated sync IO threads you're experimenting with. Otherwise, is there any appetite for a backported (and safer/sync-over-async free) version of this handshake gate for the 2.x track, or are you primarily focused on V3 for these types of core connection pipeline fixes? |
2.13.17 is very recent - no problem there; however, I was talking about the test usage of I'm definitely open to working through the issue, both in main and 2.x - however, introducing a sync-over-async into core logic is probably a little on the dangerous side, especially if that could cause a slam during connection. I'd quite like to take a step back and make sure I understand what we're fixing; if the problem is:
then rather than introducing an extra gate so that there's a second hold point, I'd lean towards "don't flag it as available prematurely" - then there's no need for the gate! |
Description
Fixes #2989 / #3085
This PR introduces an explicit, asynchronous
TaskCompletionSource<bool>handshake coordination gate within thePhysicalBridgeprocessing loop. This completely eliminates an intermittent protocol corruption race condition that occurs when the .NET ThreadPool experiences heavy starvation during rapid socket reconnection cycles.Technical Root Cause
When a multiplexed socket connection drops under thread strain, the
PhysicalBridgecorrectly initiates an asynchronous restart. However, the subsequent server handshake commands (HELLO,AUTH,CLIENT SETNAME) rely on parsing asynchronous callbacks handled downstream by thePipeReaderrunning on the shared ThreadPool.Under severe ThreadPool exhaustion, these parsing continuations can be heavily delayed. Prior to this patch, the connection state was flagged as available prematurely relative to the actual protocol acknowledgment. As a result, concurrent user application threads checking the connection status would immediately start dispatching application-level data payloads down the socket. Because these payloads interleave into the raw socket pipe before the server completely processes and acknowledges the initial
AUTHhandshake, the protocol boundaries become completely misaligned, throwing deceptiveNOAUTHorUnauthorizedexceptions and locking the bridge into an unstable retry loop.Resolution Strategy
TaskCompletionSource<bool>(_handshakeCompletion) assigned withTaskCreationOptions.RunContinuationsAsynchronouslyto isolate the lifetime of the socket connection sequence.PhysicalBridge.cs. If a data payload attempts a transmission while the state isConnectedEstablishing, the calling task cleanly executes anawait _handshakeCompletion.Task.ConfigureAwait(false);.AUTH,HELLO,CLIENT SETNAME). OnceOnFullyEstablished()is safely invoked following the final protocol validation,_handshakeCompletion.TrySetResult(true)is fired, atomically releasing any queued application tasks to send their data down a perfectly authenticated stream.Verification
ThreadPool.SetMinThreads(1, 1)) to ensure high-concurrency requests cleanly queue behind the establishing gate without dropping or triggering thread lockups.