-
-
Notifications
You must be signed in to change notification settings - Fork 251
Fix UDP socket close hangs on Windows #1246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1403,8 +1403,11 @@ async def aclose(self) -> None: | |
| pass | ||
|
|
||
| self._transport.close() | ||
| await sleep(0) | ||
| self._transport.abort() | ||
| try: | ||
| await sleep(0) | ||
| finally: | ||
| self._transport.abort() | ||
| await AsyncIOBackend.cancel_shielded_checkpoint() | ||
|
|
||
|
|
||
| class _RawSocketMixin: | ||
|
|
@@ -1687,6 +1690,11 @@ async def aclose(self) -> None: | |
| self._closed = True | ||
| if not self._transport.is_closing(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is wrong, if aclose_forcefully is called concurrently the second one will return before the socket is closed |
||
| self._transport.close() | ||
| try: | ||
| await sleep(0) | ||
| finally: | ||
| self._transport.abort() | ||
| await AsyncIOBackend.cancel_shielded_checkpoint() | ||
|
|
||
| await self._protocol.closed_event.wait() | ||
|
|
||
|
|
@@ -1737,6 +1745,11 @@ async def aclose(self) -> None: | |
| self._closed = True | ||
| if not self._transport.is_closing(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is wrong, if aclose_forcefully is called concurrently the second one will return before the socket is closed |
||
| self._transport.close() | ||
| try: | ||
| await sleep(0) | ||
| finally: | ||
| self._transport.abort() | ||
| await AsyncIOBackend.cancel_shielded_checkpoint() | ||
|
|
||
| await self._protocol.closed_event.wait() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |||||
| from pytest import FixtureRequest | ||||||
| from pytest_mock.plugin import MockerFixture | ||||||
|
|
||||||
| import anyio | ||||||
| from anyio import ( | ||||||
| BrokenResourceError, | ||||||
| BusyResourceError, | ||||||
|
|
@@ -518,6 +519,14 @@ async def test_send_after_close(self, server_addr: tuple[str, int]) -> None: | |||||
| with pytest.raises(ClosedResourceError): | ||||||
| await stream.send(b"foo") | ||||||
|
|
||||||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason to limit this to asyncio? Surely we should have the same behaviour on asyncio and trio
Suggested change
|
||||||
| async def test_aclose_forcefully(self, server_addr: tuple[str, int]) -> None: | ||||||
| stream = await connect_tcp(*server_addr) | ||||||
| sock = stream.extra(SocketAttribute.raw_socket) | ||||||
| await stream.send(b"x") | ||||||
| await anyio.aclose_forcefully(stream) | ||||||
| assert sock.fileno() == -1 | ||||||
|
|
||||||
| async def test_receive_after_peer_closed( | ||||||
| self, family: AnyIPAddressFamily, request: FixtureRequest | ||||||
| ) -> None: | ||||||
|
|
@@ -1743,6 +1752,17 @@ async def test_aclose_waits_for_fd_release( | |||||
| udp = await UDPSocket.from_socket(sock) | ||||||
| await udp.aclose() | ||||||
|
|
||||||
| @pytest.mark.skipif(sys.platform != "win32", reason="Windows only") | ||||||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) | ||||||
|
Comment on lines
+1755
to
+1756
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason to limit this to asyncio on windows? Surely this should pass on non windows and trio
Suggested change
|
||||||
| async def test_aclose_during_send(self) -> None: | ||||||
| udp = await create_udp_socket(local_host="127.0.0.1") | ||||||
| sock = udp.extra(SocketAttribute.raw_socket) | ||||||
| await udp.sendto(b"x", "127.0.0.1", 9999) | ||||||
| with fail_after(1): | ||||||
| await anyio.aclose_forcefully(udp) | ||||||
|
|
||||||
| assert sock.fileno() == -1 | ||||||
|
|
||||||
| async def test_extra_attributes(self, family: AnyIPAddressFamily) -> None: | ||||||
| async with await create_udp_socket( | ||||||
| family=family, local_host="localhost" | ||||||
|
|
@@ -1914,6 +1934,14 @@ async def test_aclose_waits_for_fd_release( | |||||
| finally: | ||||||
| peer.close() | ||||||
|
|
||||||
| @pytest.mark.skipif(sys.platform != "win32", reason="Windows only") | ||||||
| @pytest.mark.parametrize("anyio_backend", asyncio_params) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
is there a reason to limit this to asyncio on windows? Surely this should pass on non windows and trio |
||||||
| async def test_aclose_during_send(self) -> None: | ||||||
| udp = await create_connected_udp_socket("127.0.0.1", 9999) | ||||||
| await udp.send(b"x") | ||||||
| with fail_after(1): | ||||||
| await udp.aclose() | ||||||
|
|
||||||
| async def test_extra_attributes(self, family: AnyIPAddressFamily) -> None: | ||||||
| async with await create_connected_udp_socket( | ||||||
| "localhost", 5000, family=family | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably worth something in the news about fixing TCP's aclose_forcefully