Repository navigation
Conversation
…e reactor's recv buffers DecRef sends a still-connected peer a FIN and leaves the read side open until the peer closes, bounded by ReadTimeoutMs. Nobody reads the connection from then on, but the reactor still queued every delivery on it. In shared mode each one held a buffer of the reactor-wide group, so one peer that kept sending could take the whole group (RecvQueueEntries and RecvSlots are both 4096 by default) and park every other connection's recv on -ENOBUFS until it closed or the read timeout (60 s by default) ended it. In incremental mode the deliveries filled the connection's own ring and parked its recv, so the peer's close went unseen and the connection stayed open until the read timeout. #257 accepted a stalled group for a handler that is not reading yet; this one never will. Once the handler has let go (HandlerReleased), the recv completions hand each delivery's buffer straight back instead of queueing it: shared mode returns it to the group, incremental mode applies the return a reader would have made. The multishot stays armed, so the peer's close is still seen and still ends the connection. New HardeningTests: "tcp/exit: a peer still sending after its handler let go does not starve the reactor's other connections", its control "tcp/exit: control: a peer lingering silently after its handler let go leaves the others served", and "tcp/exit: a peer that overfills its let-go connection's ring is still seen to close (incremental)". Confirmed both ways: without the fix the next connection was never answered while the let-go peer kept sending, and 8 of 8 incremental connections stayed open after their peers closed (3 of 3 runs); with it all three pass (5 of 5). Dropping either half of the fix fails only that half's test.
Owner
Author
|
Bench, before/after:
No measurable cost: main's own three rounds spread 7% on this sample, and this PR sits in that range. Method: |
This branch has not been deployed
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.
A peer that kept sending after its connection's handler let go could take every recv buffer in the reactor's shared group, so the reactor's other connections stopped receiving until that peer closed or the read timeout ended it.
DecRefsends a still-connected peer a FIN and keeps the read side open until the peer closes, bounded byReadTimeoutMs(60 s by default). From then on nobody reads the connection, butOnTcpRecvCompletionSharedstill queued each delivery on it withComplete, and each one held a buffer of the reactor-wide group.RecvQueueEntriesandRecvSlotsare both 4096 by default, so one such peer could hold the whole group. Every other connection's recv then parked on -ENOBUFS, andRearmStarvedRecvswaits for a buffer to come back, which never happened. #257 accepted that one connection can stall the group while its handler isn't reading yet. A let-go connection is never read. In incremental mode the queued deliveries fill the connection's own ring and park its recv, so the peer's close goes unseen and the connection keeps its fd and buffer group until the read timeout. That is bounded, not a leak or growth, but it outlives the peer.Once
HandlerReleased, both recv completions hand the delivery's buffer straight back instead of queueing it. Shared mode returns it to the group. Incremental mode applies the same return a reader would have made (ApplyReturnIncremental). The multishot stays armed, so the peer's close is still seen and still ends the connection, andReadTimeoutMsstill bounds a peer that never closes. No new state:HandlerReleasedreads the refcount the sweep already reads for the deferred FIN. Deliveries already queued when the handler let go are still returned at teardown, as before. The check adds a load and a branch to every recv completion and hasn't been benchmarked yet.Tests
New, in
HardeningTests:tcp/exit: a peer still sending after its handler let go does not starve the reactor's other connectionstcp/exit: control: a peer lingering silently after its handler let go leaves the others servedtcp/exit: a peer that overfills its let-go connection's ring is still seen to close (incremental)Each lingering peer is served once, and reads its handler's FIN before it sends, so its handler has let go. The shared server has 16 buffers of 64 bytes and a queue of 64, so only the group can run out. The peer sends 2 KiB, then a second client asks for a response. In the incremental test each connection gets a ring of 4 × 1 KiB. Eight peers each send 8 KiB and close, and the test counts the server's open descriptors. Removing either half of the fix fails only that half's test.
All suites pass: Unit 63, E2E 255, Tls 155, Chaos 47 (and File 4).
Bench: this adds one load and a branch to every TCP recv completion, in both modes. A before/after run will follow here once the box is quiet; other suites are running on it now.