Repository navigation
pipes: the TCP pipe reader and writer park on the connection itself, so a read or flush that waits has one value-task source, not two - #399
Merged
Conversation
…so a read or flush that waits has one value-task source, not two TcpConnectionPipeReader waited on the connection the way a handler does: it awaited ReadAsync, registering on the connection's value-task source, and its caller awaited the reader's own. Every read that parked made two continuation registrations (a CAS each), two Resets, and the IValueTaskSource calls and token checks between the layers; a flush through TcpConnectionPipeWriter paid the same. The adapters now park on the connection itself. IRecvListener and IFlushListener (internal, one method each) are what the connection calls when the read or flush an adapter parked on completes; the adapter then completes the one source its caller awaits. TryReadOrPark and TryFlushOrPark, the adapters' entry points next to ReadAsync and FlushAsync, keep their arm protocol, guards and sweep clocks, with one addition on the read side: the exchange that takes the arm back decides who delivers. ReadAsync's re-check after the arm can deliver twice when a completion races it, and the delivery into the never-registered source is lost harmlessly; with the adapter called directly, both would run it. The flush side already decided through _flushArmed. The listener is stored before the arm that publishes it, and each adapter resets its own core before it parks, since off the reactor the completion can run before the park returns. ReadAsync and FlushAsync keep their code; their completion sites gain one null check. wrk on Tcp/Pipe, two runs of 12 paired rounds against main: user-mode CPU per request -11.4% and -13.3% (0.164 -> 0.145 us), lower in every round; total CPU per request and req/s flat, as 20 ns is under 1% of a request whose CPU is ~92% the kernel's. Tcp/Raw, which does not use the adapters, stays inside its noise (-1.5% and -4.3%, rounds on both sides of zero); the main-vs-main control moved +0.6% and +3.0% on Pipe. Tests (Unit, AdapterParkTests): a read and a flush an adapter parks are delivered exactly once when a close races their arming, and the reader's ReadAsync and the writer's FlushAsync made off the reactor complete when a close races them - 200,000 rounds each, each asserting that at least 1,000 closes landed while the arm was running. Red-checked one break per variant: the re-check delivering without taking the arm back (123 rounds delivered twice), the read listener stored after the arm (36 and 58 lost), the flush listener stored after the arm (6,971 and 16,220 lost), and each adapter resetting its core after it parks (15 and 36,336 lost) fail exactly the tests that cover them; the unmodified tree passes all four in three runs. Docs: the BCL bridges and the lost-wakeup notes in dev-core.
…as decided to park them, so an overlapping read or flush it refuses leaves the parked one to complete From review of the first commit. Each adapter reset its own value-task source before calling into the connection, so a second read or flush made while one was parked threw the first one's continuation away before the connection refused it - or, on a closed connection with the first flush still with the kernel, returned at once and left the first never resuming. On main the guard ran first. A flush with nothing staged likewise invalidated an earlier flush's unread result. The connection now calls IRecvListener.OnParking / IFlushListener.OnParking past every guard, right before the arm that publishes the park, and only when it does park: never for a read served at once, a refused one, a flush on a closed connection, or one with nothing staged. The adapters reset their source there. The reader's re-park from inside OnRecvReady, after a wake that brought nothing new, keeps the value task its caller already waits on. The flush listener slot is now written only when a flush is armed, so ArmFlush's nothing-staged path no longer leaves it set while the in-progress guard is already released. Tests (Unit, AdapterParkTests): a second read refused while one is parked, a second flush refused while one is parked, a flush on a closed connection while the kernel holds an earlier one, and a flush with nothing staged after an unread completed one each leave the first operation to complete or readable; a wake with nothing new parks the read again without losing its caller.
This was referenced Oct 25, 2026
MDA2AV
added a commit
that referenced
this pull request
Oct 25, 2026
…lot as the pipe adapters, so every wait is delivered exactly once and ResetRead has nothing left to do After #399 a read could wait two ways - the pipe reader through TryReadOrPark, ReadAsync through its own copy of the arm protocol - and a flush signalled either an adapter or the connection's own source, picked by a null check. There is now one way for each. The connection is its own IRecvListener and IFlushListener: ReadAsync is TryReadOrPark(this), FlushAsync and FlushIfIdleAsync arm through the same ArmFlush as TryFlushOrPark, and the connection's OnParking readies its own value task, its OnRecvReady/OnFlushDone complete it. A parked read is one slot, _readWaiter, in place of the _armed flag and a separate listener field, and a read parks in three steps: a CompareExchange claims the slot with a mark no completion delivers to, the listener readies its value task, a second CompareExchange publishes the listener. A re-check after the claim catches a completion that landed just before it; one that lands while the listener readies marks the claim, and the read serves what it queued instead of parking. A completion takes a published park back with one CompareExchange, and only the side that takes it delivers. A read refused for overlapping or racing a parked one has touched nothing of it: the claim comes before the listener's value task, which two reads through one adapter, or two of ReadAsync's, share. What that fixes, on main unless noted: - ReadAsync's race recovery took the arm back without looking at the result, so when a recv had already taken it both delivered: the caller was served at once, and the recv completed the read's value task as well. That stale completion raced the caller's ResetRead and next read - the value task came out torn, or the next read, parked for real, looked complete and the one after it was refused as already armed, or the next recv's SetResult threw on the reactor. - ResetRead reset the value task even with a read parked on it, throwing the read's continuation away. A read now readies its own value task when it parks, so ResetRead is a no-op, kept for the callers written to the old contract. - CompleteFlush let go of the in-progress guard before disarming the flush: a flush that took the guard in the gap found it still armed and was refused, and the completion could signal the listener that flush had just stored in place of the one it was completing. The completion now disarms, takes its listener, then lets go of the guard and signals. - From #399, for two reads parking at once: the one refused had stored its listener before the exchange that refused it, so the parked one's delivery could go to it instead; and it had readied its listener before that exchange too, so two reads through one adapter could reset the value task the parked one's caller waits on. Main had neither: the pipe reader parked through ReadAsync, whose arm exchange came first. Tests (Unit, WaiterSlotTests). The races run 200,000 rounds each, a thread playing the reactor, and each asserts the race ran: a raw read racing its recv completions leaves the reads after it working; two reads parking at once cannot take each other's place; a read refused for racing a park of the same listener never readies that listener; two raw reads at once: the one refused leaves the parked one's value task to complete; a flush that starts while the last one's completion is delivering is neither refused as armed nor signalled in its place. Two are deterministic: ResetRead while a raw read is parked leaves the read to complete; a completion that lands while a read readies its listener is served by that read, which then lets go of the slot. Red-checked against #399's connection code, twice: six fail - the raw read (its value task torn, once thrown on the side playing the reactor), ResetRead, two parks (15,937 and 15,452 rounds), the same listener (15,574 and 17,110), two raw reads (every round, as ReadAsync there leaves readying its value task to ResetRead) and the flush (35 and 13). Against one CompareExchange claiming and publishing the slot after the listener readies: the same listener (9,907 and 19,176) and two raw reads (5,567 and 11,120). The last test guards the claim's own path, and passes on both: it fails when a completion leaves a claim unmarked (the read parks with the wake spent) and when a read served after a mark keeps the slot (the next one is refused). This branch passes all seven, three runs. Docs: the connection section of dev-core.
MDA2AV
added a commit
that referenced
this pull request
Oct 25, 2026
…lot as the pipe adapters, so every wait is delivered exactly once and ResetRead has nothing left to do (#401) After #399 a read could wait two ways - the pipe reader through TryReadOrPark, ReadAsync through its own copy of the arm protocol - and a flush signalled either an adapter or the connection's own source, picked by a null check. There is now one way for each. The connection is its own IRecvListener and IFlushListener: ReadAsync is TryReadOrPark(this), FlushAsync and FlushIfIdleAsync arm through the same ArmFlush as TryFlushOrPark, and the connection's OnParking readies its own value task, its OnRecvReady/OnFlushDone complete it. A parked read is one slot, _readWaiter, in place of the _armed flag and a separate listener field, and a read parks in three steps: a CompareExchange claims the slot with a mark no completion delivers to, the listener readies its value task, a second CompareExchange publishes the listener. A re-check after the claim catches a completion that landed just before it; one that lands while the listener readies marks the claim, and the read serves what it queued instead of parking. A completion takes a published park back with one CompareExchange, and only the side that takes it delivers. A read refused for overlapping or racing a parked one has touched nothing of it: the claim comes before the listener's value task, which two reads through one adapter, or two of ReadAsync's, share. What that fixes, on main unless noted: - ReadAsync's race recovery took the arm back without looking at the result, so when a recv had already taken it both delivered: the caller was served at once, and the recv completed the read's value task as well. That stale completion raced the caller's ResetRead and next read - the value task came out torn, or the next read, parked for real, looked complete and the one after it was refused as already armed, or the next recv's SetResult threw on the reactor. - ResetRead reset the value task even with a read parked on it, throwing the read's continuation away. A read now readies its own value task when it parks, so ResetRead is a no-op, kept for the callers written to the old contract. - CompleteFlush let go of the in-progress guard before disarming the flush: a flush that took the guard in the gap found it still armed and was refused, and the completion could signal the listener that flush had just stored in place of the one it was completing. The completion now disarms, takes its listener, then lets go of the guard and signals. - From #399, for two reads parking at once: the one refused had stored its listener before the exchange that refused it, so the parked one's delivery could go to it instead; and it had readied its listener before that exchange too, so two reads through one adapter could reset the value task the parked one's caller waits on. Main had neither: the pipe reader parked through ReadAsync, whose arm exchange came first. Tests (Unit, WaiterSlotTests). The races run 200,000 rounds each, a thread playing the reactor, and each asserts the race ran: a raw read racing its recv completions leaves the reads after it working; two reads parking at once cannot take each other's place; a read refused for racing a park of the same listener never readies that listener; two raw reads at once: the one refused leaves the parked one's value task to complete; a flush that starts while the last one's completion is delivering is neither refused as armed nor signalled in its place. Two are deterministic: ResetRead while a raw read is parked leaves the read to complete; a completion that lands while a read readies its listener is served by that read, which then lets go of the slot. Red-checked against #399's connection code, twice: six fail - the raw read (its value task torn, once thrown on the side playing the reactor), ResetRead, two parks (15,937 and 15,452 rounds), the same listener (15,574 and 17,110), two raw reads (every round, as ReadAsync there leaves readying its value task to ResetRead) and the flush (35 and 13). Against one CompareExchange claiming and publishing the slot after the listener readies: the same listener (9,907 and 19,176) and two raw reads (5,567 and 11,120). The last test guards the claim's own path, and passes on both: it fails when a completion leaves a claim unmarked (the read parks with the wake spent) and when a read served after a mark keeps the slot (the next one is refused). This branch passes all seven, three runs. Docs: the connection section of dev-core.
Merged
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.
TcpConnectionPipeReaderandTcpConnectionPipeWriterwaited on the connection the way a handler does. A read that had to wait went through two value-task sources:TcpConnection.ReadAsync(), registering a continuation on the connection's source.So every read that parked made two continuation registrations (a CAS each), two
Resets and theIValueTaskSourceinterface calls and token checks between the layers. A flush through the writer paid the same.The adapters now park on the connection itself:
IRecvListener/IFlushListener: internal, two methods each. The connection callsOnParkingwhen it is about to park the adapter, andOnRecvReady/OnFlushDonewhen the read or flush completes. The adapter completes the one source its caller awaits.TryReadOrPark/TryFlushOrPark: the adapters' entry points, next toReadAsyncandFlushAsync. Same arm protocol, same guards, same sweep clocks.TryReadOrParkthe exchange that takes the arm back decides who delivers. The flush side already worked that way through_flushArmed.ReadAsynckeeps main's code, where both sides deliver when a completion races the re-check after the arm. That isn't as harmless as this PR first said: the second delivery can leave the connection's source looking complete to the caller's next read. It is on main, and conn: ReadAsync and FlushAsync park the connection through the same slot as the pipe adapters, so every wait is delivered exactly once and ResetRead has nothing left to do #401 fixes it.OnParking. The connection calls it past every guard, right before the arm, and only when it does park: never for a read served at once, one it refuses, a flush on a closed connection, or one with nothing staged. The listener is stored before the arm that publishes it. Off the reactor, the completion can run before the park returns.ReadAsyncandFlushAsynckeep their code; their completion sites gain one null check. The public API is unchanged.Who gets it: everything that reads or writes through these two adapters.
TcpConnectionDualPipe: GenHTTP's ioxide engine over plain TCP, and h2c on both HTTP/2 stacks.TlsConnectionDualPipeonce a connection is on kTLS, since it wraps the same reader and writer.From review
Three reviews of the first commit (its concurrency, its consumers, and a stress run) found the same regression:
ArmFlush's nothing-staged check, so that path left it set after the in-progress guard was released.The second commit fixes all three, with
OnParkingas described above. The reader's re-park from insideOnRecvReady, after a wake that brought nothing new, keeps the value task its caller already waits on.Two narrower cases remain, both when two threads park at once (a race, where the hang above is a sequence). Both are fixed in #401, which includes this PR's commits:
The reviews also found two problems on main that this PR leaves as they were. #401 fixes both:
ReadAsyncdescribed above, which the concurrency review's harness hit in 403 of 400,000 rounds as a spurious "ReadAsync already armed";CompleteFlushletting go of the in-progress guard before it disarms, so a flush taking the guard in that gap is refused as already armed.Measured
wrkonTcp/Pipe, withTcp/Rawas the control because it doesn't use the adapters:wrk -t4 -c64on other cores, 2-byte body./proc/PID/stat. It separates this change from the kernel's TCP path, which is 92–95% of a request's CPU here.Tcp/PipeTcp/PipeTcp/PipeTcp/RawTcp/RawTcp/RawOn
Tcp/Pipethe first commit saved about 20 ns of user-mode CPU per request: medians 0.164 → 0.145 µs and 0.163 → 0.143 µs, lower in all 24 rounds. The second commit, which adds theOnParkingcall, measured the same: 0.166 → 0.144 µs, lower in all 12 rounds. Throughput doesn't move, because 20 ns is under 1% of a ~2.1 µs request, inside what a single round varies.Tcp/Rawstays inside its noise, with rounds on both sides of zero.Tests
New, in
AdapterParkTests(Unit, which sees the internals). Four race an arm made off the reactor against a close made on it:The four:
park: a read an adapter parks is delivered exactly once when a close races its arming: counts deliveries per round (the park returning a snapshot, plus calls to a counting listener). Every round must add up to exactly 1.park: a flush an adapter parks is delivered exactly once when a close races its arming: the same forTryFlushOrPark.pipereader: a read from off the reactor that races the connection's close completes: through the real reader. ItsReadAsyncmust complete, and report the close.pipewriter: a flush from off the reactor that races the connection's close completes: the same through the writer'sFlushAsync.In passing runs, 8,000 to 96,000 of the 200,000 rounds overlapped, depending on the test.
Red-checked on the first commit, one break per variant:
Five more, deterministic, new in the second commit:
pipereader: a second read refused while one is parked leaves the parked one to completepipewriter: a second flush refused while one is parked leaves the parked one to completepipewriter: a flush made after the peer closed, while the kernel holds an earlier one, leaves that one to completepipewriter: a flush with nothing staged leaves an earlier flush's unread result readablepipereader: a wake with nothing new parks the read again without losing its callerRed-checked:
Operation is not valid due to the current state of the object.OnParkingresets on the re-park insideOnRecvReadytooThe existing tests cover everything else through the adapters:
PipeReaderContractTestsandRecvBufferReclaimTests.TcpConnectionDualPipe(Http, Chaos).TlsConnectionDualPipewraps these same adapters once the kernel has the connection (Tls, with thetlsmodule loaded, so none of it skipped).Docs: the BCL bridges and the lost-wakeup notes in
dev-core.All suites pass: Unit 114, E2E 326, Http 89, Tls 194, Chaos 80, File 5, Pg 7 (+11 skipped), Redis (6 skipped, 1 pending).
tlsmodule was loaded, so no Tls case skipped.Merges cleanly with the
AdvanceTochange opened alongside it (#400), and with #386, #384 and #363. Against #307 the only conflict is inHardeningTests.cs, which #307 already has with main.Not changed:
TcpConnectionStream, the userspace-TLS pipes (TlsDecryptingPipeReader,TlsEncryptingPipeWriter) and the QUIC pipe reader still chain onto their connection's value tasks. They can move to the same listeners in a follow-up.🤖 Generated with Claude Code