Fix NetworkWriter.AsyncFlushPages hanging FlushEvent waiters on send failure#1949
Fix NetworkWriter.AsyncFlushPages hanging FlushEvent waiters on send failure#1949hexonal wants to merge 5 commits into
Conversation
…failure When networkSender.SendResponse throws inside AsyncFlushPages (e.g. because the connection was disposed concurrently, such as after a cluster gossip timeout), the catch block logged the exception and disposed the handler but never invoked AsyncFlushPageCallback for that page. That callback is the only place that decrements the page-completion counter and signals FlushEvent, so any caller already blocked on flushEvent.Wait (e.g. GarnetClient.InternalExecuteNoResponse, used by cluster PUBLISH forwarding) would hang until the next gossip round disposed the whole NetworkWriter. This addresses the remaining "main thread blocking" symptom described in microsoft#1929 that is not covered by microsoft#1931, which only fixes the separate NullReferenceException in GarnetServerNode.TryClusterPublish. This change does not touch that code path and does not claim to fully resolve microsoft#1929. Adds a regression test that reproduces the hang by forcing a send failure while a thread is parked on the flush-completion event, and verifies it is released instead of only unblocking via a cancellation timeout. The test needs direct access to NetworkWriter's internals to inject the failure and observe the specific completion-event generation, so Garnet.client grants InternalsVisibleTo to Garnet.test (matching the pattern already used by other libs projects).
Replace the fixed Thread.Sleep(50) rendezvous in AsyncFlushPagesSignalsFlushEventOnSendFailure with a deterministic readiness signal: the waiter thread flips a volatile flag immediately before calling flushEvent.Wait, and the main thread spins on that flag (bounded by SpinWait, no fixed delay) before triggering the failing flush. This removes the timing assumption an independent review flagged as a source of a low-frequency flake under load, in a test whose entire purpose is proving the fix doesn't hang.
There was a problem hiding this comment.
Pull request overview
This PR fixes a client networking hang where NetworkWriter.AsyncFlushPages could swallow exceptions from INetworkSender.SendResponse without signaling FlushEvent, leaving callers blocked indefinitely (notably in cluster-related forwarding paths that synchronously wait for flush completion).
Changes:
- Ensure
AsyncFlushPagessignals completion even whenSendResponsethrows by invoking the page flush callback on the exception path. - Add a regression test that simulates a send failure and asserts
FlushEventwaiters are released. - Allow the test assembly to access
Garnet.clientinternals viaInternalsVisibleTo.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/standalone/Garnet.test/GarnetClientTests.cs | Adds a regression test that forces a send failure during AsyncFlushPages and verifies FlushEvent waiters do not hang. |
| libs/client/NetworkWriter.cs | Ensures flush completion bookkeeping runs even when SendResponse throws, avoiding indefinite waits. |
| libs/client/Garnet.client.csproj | Grants Garnet.test access to internal client types used by the new regression test. |
| logger?.LogError(ex, "Exception calling networkSender.SendResponse in AsyncFlushPages"); | ||
| networkHandler.Dispose(); | ||
| // Still signal completion for this page so FlushEvent waiters (e.g. GarnetClient | ||
| // callers blocked in InternalExecuteNoResponse) are not left hanging indefinitely. | ||
| AsyncFlushPageCallback(asyncResult); |
There was a problem hiding this comment.
@hexonal, this seems serious. Can we guarantee that AsyncFlushPages does not decrement the value twice?
In the worst case, it might put the count into a state that creates the same issue that you were trying to resolve.
There was a problem hiding this comment.
I went through this carefully because it's a fair concern. AsyncFlushPageCallback does Interlocked.Decrement(ref result.count.count), and only signals FlushEvent once that hits zero.
Looking at ClientTcpNetworkSender.SendResponse, every exception it can throw falls into one of two buckets: either it happens before the SocketAsyncEventArgs is even dispatched (throttle wait, checkout, buffer setup — no callback has run yet), or socket.SendAsync() itself throws synchronously, which per the SAEA contract means the operation was never actually queued, so Completed can never fire for it afterward. If SendAsync succeeds instead, SendResponse just returns normally and the catch block never runs for that page at all.
So there's no path where a page gets counted twice — the callback fires from exactly one place per page, either the normal completion or (only when the send never got queued) the new catch-block call, never both. I didn't just reason through this abstractly, I diffed the actual send path line by line against main to make sure I wasn't missing a partial-completion case.
| var waiterThread = new Thread(() => | ||
| { | ||
| Volatile.Write(ref aboutToWait, true); | ||
| try { flushEvent.Wait(cts.Token); } | ||
| catch (Exception ex) { waiterException = ex; } | ||
| }) |
There was a problem hiding this comment.
Good catch, fixed in c4e561d — you're right that flipping aboutToWait before actually calling Wait() still left a real window.
Went one step further than the suggested fix, though: instead of registering the wait via WaitAsync() on a background thread and blocking on the returned task there, I realized there's no need for a separate thread at all. WaitAsync() registers with the underlying SemaphoreSlim synchronously, before it returns — so the test can just call flushEvent.WaitAsync(cts.Token) on the main thread, trigger the failing flush right after (no rendezvous needed, since registration already happened), and then block on the returned Task with a timeout. That removes the aboutToWait flag, the SpinWait, and the background thread entirely, not just the race between them.
Ran it 5x in a row locally plus the full GarnetClientTests suite (20/20) to sanity-check.
|
One red job on the last run: |
Per Copilot review feedback: setting the aboutToWait flag before actually calling flushEvent.Wait() still left a narrow window where Set()/Dispose() on the underlying SemaphoreSlim could race the waiter thread's own call to Wait(), occasionally surfacing as an ObjectDisposedException instead of exercising the intended flow. Switch to flushEvent.WaitAsync(), which registers with the semaphore synchronously before returning. This closes the race entirely rather than narrowing it, and removes the need for a separate waiter thread and readiness rendezvous altogether: the test can just trigger the flush and then block on the returned Task with a timeout.
Context
This is a follow-up on the "main thread blocking / hang" symptom described in #1929. #1929 traces back to a
NullReferenceExceptioninGarnetServerNode.TryClusterPublishafter a gossip timeout, which is fixed by the (currently unmerged) maintainer PR #1931 via an!IsConnectedguard.This PR does not touch that code path and does not claim to resolve #1929 on its own. It fixes a separate bug that is reachable from the same general area (a broken/disposed cluster connection during a flush) and produces the same class of symptom — a caller thread hanging indefinitely instead of failing fast. Whether or not it is the root cause of every hang reported under #1929 I can't say with confidence; what I can say precisely is described below.
Root cause
NetworkWriter.AsyncFlushPagessends each buffered page vianetworkSender.SendResponse(...), passing aPageAsyncFlushResultas the completion context. Normally,SendResponse(synchronously or via a completion callback) eventually invokesAsyncFlushPageCallback(context), which decrementsPageAsyncFlushResult.count.countand, once it reaches zero, callsFlushEvent.Set().If
SendResponsethrows (e.g. because the underlying connection was concurrently disposed/broken — which is exactly what happens when a gossip timeout tears down a cluster connection), the existing code:logs the error and disposes the handler, but never invokes
AsyncFlushPageCallbackfor that page.FlushEventis therefore never signaled for this flush. Any thread blocked on it — most notablyGarnetClient.InternalExecuteNoResponse, whichflushEvent.Wait()s synchronously and is used by cluster PUBLISH forwarding — hangs indefinitely (or until an external timeout/cancellation, if the caller supplied one).Fix
Explicitly invoke
AsyncFlushPageCallback(asyncResult)in the catch block so the completion counter is still decremented andFlushEventstill gets signaled when a send fails, matching what would have happened on a successful send. 3-line change inlibs/client/NetworkWriter.cs.Known related limitation (not fixed here)
ClientTcpNetworkSender.SendResponsecan complete synchronously and inline (when the underlyingsocket.SendAsyncreturnsfalse), which meansAsyncFlushPageCallbackcan already be running inline underneathSendResponsewhen it throws. In the narrow case whereAsyncFlushPageCallbackitself throws (e.g. fromepoch.BumpCurrentEpoch, reachable from inside it), this fix's catch block would invokeAsyncFlushPageCallbacka second time for a result whose completion counter was already decremented once, which could corruptcount.countfor that flush batch. I have not observed this in practice and did not reproduce it — flagging it as a known, narrow, pre-existing-adjacent edge case rather than silently leaving it unaddressed. Happy to follow up separately if it's judged worth guarding against explicitly.Testing
AsyncFlushPagesSignalsFlushEventOnSendFailureintest/standalone/Garnet.test/GarnetClientTests.cs: parks a thread onflushEvent.Wait()(mirroring a caller already blocked when the connection dies), swaps in a network sender stub whoseSendResponsealways throws, then triggersAsyncFlushPagesand asserts the waiter is released rather than timing out.OperationCanceledExceptionafter a 3s cancellation timeout (a real hang). With the fix restored, it passes in under a second.InternalsVisibleToonGarnet.clientfor the test assembly (NetworkWriterisinternal sealed); added tolibs/client/Garnet.client.csprojusing the repo's existing signing key (same key already used bylibs/host/Garnet.host.csprojand others).GarnetClientTestsclass (20 tests): 8/8 full-class runs green, no flakes observed.dotnet buildsucceeds forGarnet.client.csproj,Garnet.cluster.csproj, andGarnet.test.csproj.dotnet format --verify-no-changesis clean on the changed files.NetworkWriterunit level described above, not a full cluster integration repro.