fix(iouring): restore the unconditional recv-pause state assignment (regression from #511) - #516
Closed
FumingPower3925 wants to merge 3 commits into
Closed
fix(iouring): restore the unconditional recv-pause state assignment (regression from #511)#516FumingPower3925 wants to merge 3 commits into
FumingPower3925 wants to merge 3 commits into
Conversation
…rker #511 added a re-enqueue when getCancelSQE could not obtain an SQE, so the pause would be retried. That was wrong in two ways, and it regressed main. The worker loop calls drainDetachQueue on every iteration. Re-enqueueing the conn and re-arming detachQPending therefore makes the worker drain the queue again immediately instead of reaping the completions that would free the ring -- and because the append happens while the drain iterates its swapped-out spare slice, each pass appends the SAME conn again, so the queue grows without bound for as long as the ring stays full. The symptom is the celeris#482 regression guard failing on io_uring with close-timeouts: TestBackpressurePauseDoesNotCancelInflightSend reported conns that never completed their Close handshake, on kernel 6.17 CI runners where io_uring probes at tier=optional and the ring fills more readily than on the cluster. It failed on two PRs that touch neither io_uring nor WebSockets (#513 ratelimit, #515 h1), which is what identified #511 rather than either of them as the cause. Drop the re-enqueue and keep the correctness property: still do NOT record the conn as paused when the cancel was not submitted, because its multishot recv is still kernel-armed and a resume would arm a second one. The pause is simply deferred -- recvPauseDesired stays true and recvPaused stays false, so the next drain retries. A conn under backpressure is by definition still moving data, so another drain is imminent. Until then recv stays armed, which costs throughput on one conn but is always safe, unlike a phantom pause.
Contributor
Author
|
Cluster verification, msa2-server (amd64, kernel 7.0.0-30), the #482 regression guard with this fix, 3 consecutive runs: Zero close-timeouts. |
My #511 stopped marking a conn paused when the cancel SQE could not be obtained. With no retry that means the middleware's requested pause NEVER takes effect: recvPauseDesired stays true, recvPaused stays false, and PauseRecv only enqueues on the false->true transition, so nothing tries again. Under sustained backpressure the peer keeps filling a buffer nobody drains and the conn hangs -- which regressed the celeris#482 guard with close-timeouts on kernel 6.17 CI runners, where io_uring probes at tier=optional and the ring fills far more readily than on the cluster at tier=high. The first attempt at a fix re-enqueued the conn instead. That was worse: the worker loop drains the queue every iteration, so it spun on that conn rather than reaping the completions that would free the ring, and each pass appended the same conn again. Restore the shipped semantics -- mark the conn paused unconditionally -- and keep the one genuine improvement from #511: getCancelSQE submits and retries once, so the cancel actually lands far more often than with a bare GetSQE. The residual risk is the one #482 documented: on a full ring the recv is still kernel-armed while we record it as paused, so a later resume can arm a second multishot recv. That is pre-existing, rare, and strictly less harmful than a pause that never happens at all.
FumingPower3925
changed the title
revive's empty-block fired on the sqe == nil arm, which after restoring the unconditional pause assignment held only the explanation. Invert to `if sqe := w.getCancelSQE(); sqe != nil` and keep the reasoning above the branch, where it explains the unconditional assignment rather than an empty arm. No behaviour change.
Contributor
Author
|
Superseded by #518, which reverts #511 verbatim and is green on CI. #516 restored only the unconditional |
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.
Fixes a regression I introduced in #511, currently on main.
What #511 got wrong
#511 stopped marking a conn paused when the cancel SQE could not be obtained. The intent was to avoid recording a pause that had not actually been submitted — but with no retry, that means the middleware's requested pause never takes effect:
recvPauseDesiredstays true,recvPausedstays false, andPauseRecvonly enqueues on thefalse→truetransition, so nothing tries again.Under sustained backpressure the peer keeps filling a buffer nobody drains and the conn hangs. That regressed the #482 guard with close-timeouts on kernel 6.17 CI runners, where io_uring probes at
tier=optionaland the ring fills far more readily than on the cluster attier=high.What my first attempt got wrong
The first version of this PR re-enqueued the conn to retry. That was worse: the worker loop drains the queue every iteration, so it spun on that conn instead of reaping the completions that would free the ring — and because the append happens while the drain iterates its swapped-out spare slice, each pass appended the same conn again.
The fix
Restore the shipped semantics — mark the conn paused unconditionally — and keep the one genuine improvement from #511:
getCancelSQEsubmits and retries once, so the cancel actually lands far more often than with a bareGetSQE.The residual risk is the one #482 already documented: on a full ring the recv is still kernel-armed while we record it as paused, so a later resume can arm a second multishot recv. That is pre-existing, rare, and strictly less harmful than a pause that never happens at all.
How the cause was identified
The guard failed on three PRs that touch neither io_uring nor WebSockets, and on main itself:
The transition sits exactly at #511. I initially read the first failure as CI flakiness because main looked green at the time; that was wrong, and the second and third occurrences are what forced the correct diagnosis.
Note the cluster passes this test either way at
tier=high— only the constrained runner exposes it, which is why local verification was not sufficient here.