Fix UDP socket close hangs on Windows - #1246
Conversation
|
Can you add a test that: sock = udp.extra(SocketAttribute.raw_socket)
await anyio.aclose_forcefully(udp)
assert sock.fileno() == -1 |
|
Added. The test initially left the fd open, so abort now runs in finally and gets a shielded checkpoint. The focused UDP tests pass. |
|
Do SocketStreams have the same problem with aclose_forcefully? |
for more information, see https://pre-commit.ci
|
Yes, when a send is in flight. I added the same cleanup pattern and a TCP regression test. |
| with pytest.raises(ClosedResourceError): | ||
| await stream.send(b"foo") | ||
|
|
||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
There was a problem hiding this comment.
is there a reason to limit this to asyncio? Surely we should have the same behaviour on asyncio and trio
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
| @pytest.mark.skipif(sys.platform != "win32", reason="Windows only") | ||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
There was a problem hiding this comment.
is there a reason to limit this to asyncio on windows? Surely this should pass on non windows and trio
| @pytest.mark.skipif(sys.platform != "win32", reason="Windows only") | |
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
| peer.close() | ||
|
|
||
| @pytest.mark.skipif(sys.platform != "win32", reason="Windows only") | ||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
There was a problem hiding this comment.
| @pytest.mark.parametrize("anyio_backend", asyncio_params) |
is there a reason to limit this to asyncio on windows? Surely this should pass on non windows and trio
| module name. (The default name for a task spawned with ``TaskGroup.start_soon`` or | ||
| ``TaskGroup.start`` typically includes the module name.) | ||
| (`#1234 <https://github.com/agronholm/anyio/pull/1234>`_; PR by @gschaffner) | ||
| - Fixed UDP socket closing hanging on Windows if a datagram send was still in flight |
There was a problem hiding this comment.
probably worth something in the news about fixing TCP's aclose_forcefully
| @@ -1687,6 +1690,11 @@ | |||
| self._closed = True | |||
| if not self._transport.is_closing(): | |||
There was a problem hiding this comment.
I think this is wrong, if aclose_forcefully is called concurrently the second one will return before the socket is closed
| @@ -1737,6 +1745,11 @@ | |||
| self._closed = True | |||
| if not self._transport.is_closing(): | |||
There was a problem hiding this comment.
I think this is wrong, if aclose_forcefully is called concurrently the second one will return before the socket is closed
| async def aclose(self) -> None: | ||
| self._closed = True | ||
| if not self._transport.is_closing(): | ||
| try: | ||
| self._transport.write_eof() | ||
| except OSError: | ||
| pass | ||
|
|
||
| self._transport.close() | ||
| await sleep(0) | ||
| self._transport.abort() | ||
| try: | ||
| await sleep(0) | ||
| finally: | ||
| self._transport.abort() | ||
| await AsyncIOBackend.cancel_shielded_checkpoint() |
There was a problem hiding this comment.
same here with concurrent aclose_forcefully I think we want this code:
async def aclose(self) -> None:
self._closed = True
if not self._transport.is_closing():
try:
self._transport.write_eof()
except OSError:
pass
self._transport.close()
try:
await sleep(0)
finally:
self._transport.abort()
await AsyncIOBackend.cancel_shielded_checkpoint()The previous versions queued sends and then entered a task group, but start_soon() only schedules — the children first run when the host yields at __aexit__, and one event loop iteration is all the proactor needs to reap the overlapped send. close() then took the no-pending-write path and the tests passed on Windows for the wrong reason. anyio puts send()'s checkpoint before transport.sendto(), so the operation is only still in flight if nothing yields afterwards. Drop the pointless 10x send loop (each iteration yielded, so the buffer drained every time) and close directly after a single send, as PR agronholm#1246's own reproducer does. Adds a concurrent variant gated on an Event so the second closer only runs once the first has yielded inside aclose(), which is where its unshielded wait on closed_event is exposed. Neither PR covers that caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NOTE Erasing or replacing the contents of this template will result in your pull
request being summarily closed without consideration!
Changes
Fixes #1237.
On Windows Proactor event loops, closing a UDP socket while a datagram send is in flight can leave
connection_lost()unscheduled and makeaclose()wait indefinitely. Follow the transport close with an event-loop turn andabort(), matching the existing stream cleanup path, so the protocol close event is always delivered.The regression tests cover connected and unconnected UDP sockets on Windows.
Checklist
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.