Repository navigation
IO: handle Tcp.Close while a ConfirmedClose is draining - #8636
Merged
Aaronontheweb merged 4 commits intoSep 24, 2026
Merged
Aaronontheweb merged 4 commits into
Aaronontheweb merged 4 commits into
Conversation
) Once ClosingBehaviour entered a ConfirmedClose, a later Tcp.Close was silently dropped, so the connection waited forever for the peer's FIN. Artery's inbound connections hit this: they use halfClose + Source.Empty, so they send ConfirmedClose right after accept, and the read-side kill switch cancellation on shutdown sends Close. Tcp.Close now upgrades an in-flight ConfirmedClose to a full close: stop waiting for the peer's FIN, finish as soon as our own output has drained, and close the socket gracefully (reusing akkadotnet#8630's PostStop path) instead of aborting. Every close commander involved - the original ConfirmedClose sender and the Close sender - gets notified with Closed, in addition to the registered handler.
…ing ConfirmedClose PostStop took the graceful CloseAsync path whenever a Close had upgraded an in-flight ConfirmedClose, even if the close actually ended some other way (Aborted, ErrorClosed, or the handler dying before the upgrade ever drained). In that last case CloseAsync awaits a write pump nothing else unblocks, leaking the socket instead of aborting it as before this change. Key the graceful path on the final close event instead. Also simplifies the upgrade state to a single _fullCloseCommander field, trims several restating comments, and corrects a PostStop comment and the TryFinishClose doc to not claim the upgraded close guarantees a clean drain - unread inbound data can still turn it into a reset, same as a plain Close.
Aaronontheweb
enabled auto-merge (squash)
September 24, 2026 21:02
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Sep 24, 2026
… Unbind The maintainer's review: two materializers split the termination guards for no real benefit, especially now that akkadotnet#8636 makes Tcp.Close finish a draining ConfirmedClose -- Artery's inbound connections (halfClose + Source.Empty, so they ConfirmedClose right at accept) close promptly on the read-side kill-switch cancellation, same as outbound. Reverts _outboundMaterializer, the two-arg CreateSystemMaterializer, and the IsTransportTerminating/IsStreamSupervisorTerminating changes back to dev; every Run() is back on _materializer. The two tests that reflect into the materializer field (ArteryOutboundMaterializeDuringCoordinatedShutdownSpec, ArteryShutdownSystemMessageAckRaceSpec) are back to dev's version too. Shutdown() order: flush, drain to Dropped, and the kill switch are unchanged. Unbind now runs before the new wait (renamed WaitForStreamsToStopAsync, polling _materializer's supervisor), so the listener's own children are gone before that poll starts; then _materializer.Shutdown(). Still one CancellationTokenSource, created once, spanning the flush wait and the stream-stop wait -- no second timeout. Kept: halfClose: false on every outbound connection, the TcpStages null-guard fix and its regression test, and the ArteryShutdownFlushSpec diagnostics (Stopwatch bound, missing-range reporting, Dropped printed before asserting, senderSys test-output logging, narrow XunitException catch) plus the idle-peer test.
Aaronontheweb
added a commit
to Aaronontheweb/akka.net
that referenced
this pull request
Sep 24, 2026
…st diagnostics WaitForStreamsToStopAsync asked a supervisor without checking whether it was already gone: the automatic Shutdown() CoordinatedShutdown runs after a test already called it explicitly would ask a supervisor the first call's Materializer.Shutdown() had already killed, wait out the whole deadline, and falsely log "still running ... force-stopping". Guard on IsShutdown, and narrow the catch to AskTimeoutException (the only exception it actually catches). Also: if the deadline expired before the ask ever ran (e.g. Unbind() used the whole bound), the final log said "last count: unknown" without checking. The ask now uses a fixed per-iteration timeout instead of racing the shared token, so it always gets one honest count; the token is checked separately to decide whether to keep polling. Corrected comments that credited inbound streams stopping to akkadotnet#8636: it's Unbind() stopping the TcpListener, which stops its connection-actor children, which each inbound stage's own Watch(_connection) turns into a prompt failure -- independent of akkadotnet#8636. ArteryShutdownFlushSpec: the idle-peer test now asserts the "still running" INFO line is NOT logged (the real regression guard, and coverage for the fix above) instead of only a wall-clock bound; the elapsed check is timed inside the EventFilter's own action so its confirm-absence wait doesn't inflate it. Dropped the missing-range reporting, the dropped-message snapshot/printing, and the custom exception catch in the other test -- back to one overall deadline and a plain assertion. Trimmed the TcpStages.cs and TcpSpec.cs null-guard comments, and fixed the stale _isShutdown field comment claiming Shutdown() never calls _materializer.Shutdown() (it does, deliberately, after WaitForStreamsToStopAsync).
Aaronontheweb
added a commit
that referenced
this pull request
Sep 25, 2026
…se (#8628) * Akka.Streams: fix a null-connection NRE in CloseConnectionUpstreamFinished TcpConnectionStage.TcpStreamLogic.CloseConnectionUpstreamFinished's "just Close now" branch (taken whenever half-close is off, or the read side already closed) called _connection.Tell(...) unconditionally. If upstream finishes before an OUTBOUND connect completes -- e.g. an already-completed/empty upstream source -- the stage is still in its Connecting state and _connection is null, so this threw a NullReferenceException instead of completing the stage. The half-close branch already had the same null-safe CompleteStage() fallback; this just brings the other branch in line with it. Adds a deterministic regression test: with halfClose off, upstream finishing always takes this branch, and Source.Empty completes its upstream in the SAME synchronous interpreter batch as the stage's own PreStart (which, for an outbound role, only sends Tcp.Connect and returns -- Tcp.Connected can only arrive later via an actual actor mailbox round-trip), so _connection is deterministically still null every run. The materialized connection task fails with "Connection failed" either way (PostStop's own TrySetException runs whether the stage completed gracefully or crashed), so the test instead asserts on what actually differs: the GraphInterpreter logs any exception a stage callback throws at Error before failing the stage, so a caught-and-logged NullReferenceException is exactly what disappears with this fix. Verified by temporarily reverting the guard: the test fails with the expected NullReferenceException, and passes once the guard is restored. * Akka.Remote: back to one materializer; wait for streams to stop after Unbind The maintainer's review: two materializers split the termination guards for no real benefit, especially now that #8636 makes Tcp.Close finish a draining ConfirmedClose -- Artery's inbound connections (halfClose + Source.Empty, so they ConfirmedClose right at accept) close promptly on the read-side kill-switch cancellation, same as outbound. Reverts _outboundMaterializer, the two-arg CreateSystemMaterializer, and the IsTransportTerminating/IsStreamSupervisorTerminating changes back to dev; every Run() is back on _materializer. The two tests that reflect into the materializer field (ArteryOutboundMaterializeDuringCoordinatedShutdownSpec, ArteryShutdownSystemMessageAckRaceSpec) are back to dev's version too. Shutdown() order: flush, drain to Dropped, and the kill switch are unchanged. Unbind now runs before the new wait (renamed WaitForStreamsToStopAsync, polling _materializer's supervisor), so the listener's own children are gone before that poll starts; then _materializer.Shutdown(). Still one CancellationTokenSource, created once, spanning the flush wait and the stream-stop wait -- no second timeout. Kept: halfClose: false on every outbound connection, the TcpStages null-guard fix and its regression test, and the ArteryShutdownFlushSpec diagnostics (Stopwatch bound, missing-range reporting, Dropped printed before asserting, senderSys test-output logging, narrow XunitException catch) plus the idle-peer test. * Fix a second-Shutdown hang, get an honest count, trim comments and test diagnostics WaitForStreamsToStopAsync asked a supervisor without checking whether it was already gone: the automatic Shutdown() CoordinatedShutdown runs after a test already called it explicitly would ask a supervisor the first call's Materializer.Shutdown() had already killed, wait out the whole deadline, and falsely log "still running ... force-stopping". Guard on IsShutdown, and narrow the catch to AskTimeoutException (the only exception it actually catches). Also: if the deadline expired before the ask ever ran (e.g. Unbind() used the whole bound), the final log said "last count: unknown" without checking. The ask now uses a fixed per-iteration timeout instead of racing the shared token, so it always gets one honest count; the token is checked separately to decide whether to keep polling. Corrected comments that credited inbound streams stopping to #8636: it's Unbind() stopping the TcpListener, which stops its connection-actor children, which each inbound stage's own Watch(_connection) turns into a prompt failure -- independent of #8636. ArteryShutdownFlushSpec: the idle-peer test now asserts the "still running" INFO line is NOT logged (the real regression guard, and coverage for the fix above) instead of only a wall-clock bound; the elapsed check is timed inside the EventFilter's own action so its confirm-absence wait doesn't inflate it. Dropped the missing-range reporting, the dropped-message snapshot/printing, and the custom exception catch in the other test -- back to one overall deadline and a plain assertion. Trimmed the TcpStages.cs and TcpSpec.cs null-guard comments, and fixed the stale _isShutdown field comment claiming Shutdown() never calls _materializer.Shutdown() (it does, deliberately, after WaitForStreamsToStopAsync). * Artery shutdown wait: a slow supervisor reply retries instead of ending the wait
This was referenced Sep 25, 2026
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
1 participant
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.
Closes #8634
Problem
Once
TcpConnection.ClosingBehaviourentered aConfirmedClose, it only handledAbort- a laterTcp.Closewas silently dropped, so the connection waited forever for the peer's FIN.Artery's inbound connections hit this directly: they use
halfClose: trueand writeSource.Empty, so they sendConfirmedCloseright after accept. On shutdown, the kill switch cancels the read side, the TCP stage sendsTcp.Close, and it went nowhere.This intentionally differs from JVM Akka, which also ignores
Closewhile closing - on .NET, dropping it produces an indefinitely-stuck connection, which is worse than upgrading it to a full close.Fix
In
ClosingBehaviour, aTcp.Closethat arrives while aConfirmedCloseis still in flight now upgrades it to a full close instead of being dropped:_outputShutdown- our bytes reached our kernel and the FIN was queued): immediately if that already happened, otherwise once the in-flightShutdownAsync'sTransportOperationCompletedarrives. This is not a guarantee the peer read everything - unread inbound data can still turn the close into a reset on the wire, same as a plainClose.Abort/RST, but only when the close actually finishes asClosed. This reuses the gracefulCloseAsyncfallback IO: fix Tcp.ConfirmedClose racing the peer's FIN #8630 added toPostStopfor a completedConfirmedClose- that path also only ever ranShutdownAsync(half-close, socket left open) and needsCloseAsyncto actually close it.PostStop's condition covers the upgraded case only when_closeInformation.ClosedEvent is Closed; if the close instead ends some other way (Aborted,ErrorClosed, or the handler dying before the upgrade ever drains), that path's own handling already dealt with the transport, andPostStopfalls back toAbort()as it always did - enteringCloseAsynca second time there would await a write pump nothing else can ever unblock, leaking the socket.ConfirmedClosesender and the laterClosesender - getsClosed, in addition to the registered handler (which is always notified regardless). This mirrors the existing single-close-command contract (sender + handler both notified) instead of silently dropping the earlier commander when a second close command shows up; the notification set naturally collapses to oneTellwhen it's the same actor sending both, which is the common case (e.g. a stream stage issuingConfirmedClosethen laterClosevia the sameStageActorref).ConfirmedClosewhile already closing remains a no-op (unchanged - there's still no handler registered for it inClosingBehaviour).Abortbehavior is unchanged.TryFinishClose's existing both-flags logic (_outputShutdown+_peerClosed) for a normal, non-upgradedConfirmedCloseis untouched, as is the regularClosepath (_outputShutdown+_readPumpCompleted).Tests
TcpConfirmedCloseSpec(extends IO: fix Tcp.ConfirmedClose racing the peer's FIN #8630's raw-socket-peer style):ConfirmedClosethenClosewhile the peer stays open and never sends a FIN (exercises the "already drained" branch); andClosearriving while output is still draining against a non-reading peer (exercises the "wait for drain" branch). Both assert the peer receives every byte and a clean EOF, no reset.Closeupgrades aConfirmedClosethat's still draining against a non-reading peer, then the handler dies before the drain ever finishes. Asserts the connection resets or closes promptly instead of the socket being left open indefinitely.TcpSpec(Akka.Streams.Tests): an incoming connection withhalfClose: trueandSource.Empty, whose read side is cancelled viaTestSubscriber.Probe.CancelAsync()while a raw client stays fully open. A read-only assertion can't tell a fixed close apart from a connection stuck open forever (both look like a clean EOF), so the test also has the client write a byte afterward and waits (bounded) for the OS to reset it once the server has actually closed its socket. Wrapped inAssertAllStagesStoppedAsync.All new/extended tests were confirmed to fail (hang, timeout, or leak) against the corresponding unfixed code before applying each fix.
Validation
dotnet build -warnaserrorforAkkaandAkka.TestsAkka.TestsIOnamespace: 77/77 passingAkka.Streams.TestsTcpSpec: 21/21 passing (run multiple times)Akka.Remote.TestsArterynamespace: 285/285 passingAkka.API.Tests: 23/23 passing (no public API surface changed -TcpConnectionis internal)