call set_write_buffer_limits(0) for UDPSockets so that send returns once data hits the kernel - #1294
call set_write_buffer_limits(0) for UDPSockets so that send returns once data hits the kernel#1294graingert wants to merge 8 commits into
Conversation
e07f665 to
f460010
Compare
| ) | ||
| addr = cast(IPSockAddrType, peer_path) | ||
| with peer: | ||
| async with await UDPSocket.from_socket(sock) as udp: |
There was a problem hiding this comment.
Am I reading this right? You're making an UDPSocket backed by an underlying UNIX socket?
There was a problem hiding this comment.
Yes it's the only way to get backpressure on local Datagram sockets. UDP sockets just drop packets rather than raising BlockingIOError
fallenmi
left a comment
There was a problem hiding this comment.
The zero high-water mark pauses the protocol only after asyncio has already buffered the first refused datagram, so the send() that failed to reach the OS still returns successfully. UDPSocket.send() waits on write_event before transport.sendto(), but it never waits again after sendto() synchronously invokes pause_writing() on the would-block path. Only the following call observes the cleared event.
A deterministic transport double matching _SelectorDatagramTransport.sendto()'s BlockingIOError path produced this on the exact commits:
base ae250440: send_returned=True os_accepted=False high=65536 write_paused=False
head f460010e: send_returned=True os_accepted=False high=0 write_paused=True
The new regression calls send() twice and asserts that the pair remains incomplete, which hides the first call's false completion. Please make each call wait until its own buffered datagram has been handed to the OS, and cover the would-block case with a single-send assertion.
Review prepared with Codex; the exact-base/head behavior above was reproduced locally.
What do you make of this @graingert ? |
|
It's basically saying I need to move the |
|
OK. While at it, I think we have too many checkpoints there. The first one should probably be |
|
I've left the checkpoints as is for a future PR |
fallenmi
left a comment
There was a problem hiding this comment.
The post-send wait fixes the previous false completion, but removing the pre-send wait introduces a cancellation/backpressure hole. If a would-block send is cancelled while awaiting write_event, the send guard is released while asyncio is still paused and its buffer still contains that datagram. The next send() now calls transport.sendto() before waiting; _SelectorDatagramTransport sees the non-empty buffer and appends the second datagram without attempting the OS. Repeated cancellation/retry can therefore grow the transport buffer despite the zero high-water mark.
A deterministic transport matching that CPython path produced this on exact head 1ccfe0ae for both connected and unconnected sockets:
cancel first while paused; start second before resume
send_calls=[first, second] queued=[first, second] write_event_set=False
Keeping the pre-send write_event.wait() in addition to the new post-send wait yields send_calls=[first] queued=[first] until resume_writing(). Please add a regression that cancels a paused send, starts the next send, and asserts that the second datagram does not reach the transport before resume.
Review prepared with Codex; exact current-head behavior and the minimal pre+post wait were reproduced locally, and the focused socket suite passed (232 passed, 16 platform skips).
A send() cancelled while blocked on the post-send wait leaves its datagram in the asyncio transport's buffer with the protocol still paused. Without a wait before sendto(), the next send() hands its datagram straight to the transport, which appends it to that non-empty buffer without ever trying the OS, so repeated cancellation grows the buffer despite the zero high water mark. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resolved |
|
I noticed SocketStream has the same issue #1299 |
fallenmi
left a comment
There was a problem hiding this comment.
The new head resolves my cancellation/backpressure blocker. Restoring the pre-send wait prevents a send following a cancelled paused send from being appended to asyncio’s transport buffer, while the retained post-send wait still gates completion of the current datagram. The connected and unconnected regressions cover both paths.
On exact head ae7b7d7, the deterministic transport oracle now holds at send_calls=[first] queued=[first] before resume for both socket types. The focused UDP suite passed locally (232 passed, 32 platform skips), and the current exact-head upstream matrix is 20/20 green.
Reviewed with OpenAI Codex assistance; exact current-head behavior was reproduced locally.
|
There is an excessive amount of added commentary here, no doubt from AI. Are these comments absolutely crucial, or could some of them be removed outright or at least trimmed down? |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ff3274f to
6b1ab42
Compare
|
I really wish people didn't force push changes to PRs. It prevents me from seeing clearly what changes they made in response to my comments. |
|
ah sorry about the force push |
|
Can you explain to me why we now have THREE potential checkpoints in a single |
As @fallenmi pointed out there can be a previously queued datagram due to a cancelled send call. It won't checkpoint if the event is set |
|
And if it does cfheckpoint, we have two checkpoints in a row for no reason. I'd really like to sort out the checkpointing here. Only a single checkpoint should be hit per |
|
We need to hit at least two if there's a cancelled send |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
About that – is there any pointing in reacting to cancellation after the send has already been done? Or is this about a native cancellation overriding our shielding? |
|
We don't want to shield the send wait and unfortunately we can't undo what's already been queued for write so we're a bit stuck when using Transports and Protocols |
Which wait are you talking about? The one before or after the actual sending? |
|
I was thinking that if we shield the latter send, wouldn't that remove the need for the first wait? At least unless native cancellations mess up the equations. |
|
Your latest change is a step in the direction I was looking for. Can we also skip the full checkpoint and do |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
yep, just pushed a fix like that |
|
I wasn't expecting that newly added |
|
I've also applied the same changes to #1299 |
|
Maybe it's best we hash out the changes here before you change the other PR. |
|
do we want a schedule point if we raise ClosedResourceError or BrokenResourceError? I think we do. Happy to remove the try/finally if you want |
I don't think we do. |
|
trio calls cancel_shielded_checkpoint on closed sockets see https://github.com/python-trio/trio/blob/b3fd421ab1ac7830ed7440b85331e4ae2ccaaba9/src/trio/_socket.py#L61-L90 |
So it seems, although they do it conditionally based on the exception type. I would love to understand why they do this. It's not 100% clear that we should do this too. |
|
I think we could just add that later if there's a good reason, but right now it just feels wrong. |
|
We have plenty of coroutine functions that do validation first, and don't checkpoint on errors. It seems wrong to make an exception to that norm here without a pressing reason. |
|
Apart from this, the main code looks good. I still have a problem with the way this is tested, so I'll focus on that next. |
Unlike Trio, which runs a cancel_shielded_checkpoint() on the way out of a failed socket call, the closed and broken paths now only get the checkpoint_if_cancelled() at the top. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Yep, it looks good now. I'll take a break and come back later to check if I can find another way to test this. |
fallenmi
left a comment
There was a problem hiding this comment.
The five commits since my previous approval preserve the cancellation/backpressure guarantee while reducing unnecessary checkpoints. Both UDP send paths still wait before touching the transport when a cancelled send left a datagram buffered, and still wait after sendto() when the OS refused the current datagram. The new checkpoint_if_cancelled() / cancel_shielded_checkpoint() placement also avoids sending from an already-cancelled call while retaining a mandatory yield after an immediate successful send.
On exact head 316c50ccbc08262f972c95edce8a53f6a439c878, a source-extracted deterministic oracle passed six control-flow cases for each connected and unconnected socket (12/12), including cancelled blocked send followed by another send, pre-existing backpressure, immediate success, send failure, closed resource, and pre-cancellation. The current merge da8206a771412375a669f48205e4d87870eb42f3 contains identical ASTs for both send methods and the two checkpoint primitives.
The exact merge workflow passed all 24 targeted UDP backpressure variants and its full Linux job completed with 3,081 tests passed. The live exact-head gate is 18/18 check runs, 1/1 suite, 1/1 workflow, and 2/2 legacy statuses successful.
Disclosure: I used OpenAI Codex to inspect this exact revision and current merge, run the focused source-level oracle, and verify the workflow evidence, interactions, policy, and CI. I verified the evidence and conclusion before submission.
NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!
Changes
Changed the asyncio backend to set the write buffer high water mark to 0 on UDP sockets (both connected and unconnected), so that
send()waits until the datagram has actually been passed to the operating system instead of letting the transport buffer itChecklist
If this is a user-facing code change, like a bugfix or a new feature, please ensure that
you've fulfilled the following conditions (where applicable):
tests/) which would fail without your patchdocs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).If this is a trivial change, like a typo fix or a code reformatting, then you can ignore
these instructions.
Updating the changelog
If there are no entries after the last release, use
**UNRELEASED**as the version.If, say, your patch fixes issue #123, the entry should look like this:
If there's no issue linked, just link to your pull request instead by updating the
changelog after you've created the PR.