-
-
Notifications
You must be signed in to change notification settings - Fork 250
Fix SocketStream.send() writing to a paused transport after a cancelled send #1299
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
544f230
8f3031c
8943f33
52de4fc
26377b0
b7e8c97
31b87e0
654e513
443572b
9e7558d
fb4d188
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 |
|---|---|---|
|
|
@@ -251,6 +251,62 @@ async def test_extra_attributes( | |
| assert stream.extra(SocketAttribute.remote_address) == server_addr | ||
| assert stream.extra(SocketAttribute.remote_port) == server_addr[1] | ||
|
|
||
| async def test_cancelled_send_does_not_send_the_next_one( | ||
| self, server_sock: socket.socket, server_addr: tuple[str, int] | ||
| ) -> None: | ||
| """ | ||
| Handing data to a paused transport merely appends it to the write buffer, from | ||
| where it is delivered anyway, so a cancelled ``send()`` must not have done so. | ||
| """ | ||
| payload = b"a" * 8 * 1024 * 1024 | ||
| async with await connect_tcp(*server_addr) as stream: | ||
| client, _ = server_sock.accept() | ||
| with client: | ||
| client.setblocking(False) | ||
|
|
||
| async def send_and_cancel(item: bytes) -> None: | ||
| async with create_task_group() as tg: | ||
| tg.start_soon(stream.send, item) | ||
| await wait_all_tasks_blocked() | ||
| tg.cancel_scope.cancel() | ||
|
|
||
| # Back the connection up, and then soak up any room that the peer's | ||
| # acknowledgements may have reopened in the meantime, so that the OS | ||
| # cannot take another byte. Nothing is read from the peer until further | ||
| # down, so the connection stays that way. | ||
| await send_and_cancel(payload) | ||
| await send_and_cancel(payload) | ||
|
|
||
| # On Windows, a transport can be genuinely backed up without its | ||
| # pause_writing() having fired yet: that only happens as a side effect | ||
| # of the next write() call discovering it, by which point that call's | ||
| # own data is already appended to the write buffer. Spend that one on | ||
| # a throwaway payload so the transport is *known* paused going into | ||
| # the next send() below, before it ever calls write() again. | ||
| await send_and_cancel(b"r" * 64) | ||
|
|
||
| # Now that the transport is known paused, the OS still never accepted | ||
| # any of this, so none of it may reach the peer | ||
| await send_and_cancel(b"c" * 64) | ||
|
|
||
| # Drain the peer until a final, uncancelled send() has arrived; data | ||
| # that a cancelled send() wrongly handed over would arrive first | ||
|
Comment on lines
+273
to
+293
Owner
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 don't think a mere test warrants this many lines of commentary. Would it be possible to simplify this test with mocks instead of relying on whimsy OS-level behavior? |
||
| received = bytearray() | ||
| arrived = False | ||
| with fail_after(60): | ||
|
Owner
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. This is a no-op given the 20 second default timeout we have for tests. |
||
| async with create_task_group() as tg: | ||
| tg.start_soon(stream.send, b"z" * 64) | ||
| while not arrived: | ||
| try: | ||
| data = client.recv(65536) | ||
| except BlockingIOError: | ||
| await wait_readable(client) | ||
| else: | ||
| received += data | ||
| arrived = b"z" in data | ||
|
|
||
| assert b"c" not in received | ||
|
|
||
| async def test_send_receive( | ||
| self, server_sock: socket.socket, server_addr: tuple[str, int] | ||
| ) -> None: | ||
|
|
||
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.