Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Close client connections when writer is unexpectedly cancelled #8041

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Close client connections when writer is unexpectedly cancelled #8041

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bd250d
Close client connections when writer is unexpectedly cancelled
bdraco Jan 20, 2024
8fdc7ed
raise if we did not cancel the writer
bdraco Jan 20, 2024
9160f71
naming
bdraco Jan 20, 2024
ba23b17
pass it since it can be non otherwise
bdraco Jan 20, 2024
31f7664
legacy
bdraco Jan 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 16 additions & 2 deletions aiohttp/client_reqrep.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern that occurred to me is what if this is part of a task that is being cancelled by the user? In that case we don't want to suppress.

Ideally, if we can detect if the cancellation is from our client timeout or similar, we'd suppress it, otherwise we'd reraise it. Or something along those lines...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, but might also need something to handle ClientTimeout. Can't remember exactly how the logic works, but if we hit the timeout at this point, then we probably don't actually want to time out, as we already received the response before the timeout.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think thats handled by

aiohttp/aiohttp/client_reqrep.py

Line 570 in 9e14ea1

if exc.errno is None and isinstance(exc, asyncio.TimeoutError):

Expand Up @@ -1008,7 +1008,14 @@

async def _wait_released(self) -> None:
if self._writer is not None:
await self._writer
try:
await self._writer
except asyncio.CancelledError:

Check warning on line 1013 in aiohttp/client_reqrep.py

View check run for this annotation

Codecov / codecov/patch

aiohttp/client_reqrep.py#L1013 

Added line #L1013 was not covered by tests
# Writer was cancelled, ensure the connection is closed
# since we won't be able to write the rest of the body
# we need to close the connection since we can't reuse it
self.close()
return

Check warning on line 1018 in aiohttp/client_reqrep.py

View check run for this annotation

Codecov / codecov/patch

aiohttp/client_reqrep.py#L1017-L1018 

Added lines #L1017 - L1018 were not covered by tests
self._release_connection()

def _cleanup_writer(self) -> None:
Expand All @@ -1025,7 +1032,14 @@

async def wait_for_close(self) -> None:
if self._writer is not None:
await self._writer
try:
await self._writer
except asyncio.CancelledError:

Check warning on line 1037 in aiohttp/client_reqrep.py

View check run for this annotation

Codecov / codecov/patch

aiohttp/client_reqrep.py#L1037 

Added line #L1037 was not covered by tests
# Writer was cancelled, ensure the connection is closed
# since we won't be able to write the rest of the body
# we need to close the connection since we can't reuse it
self.close()
return

Check warning on line 1042 in aiohttp/client_reqrep.py

View check run for this annotation

Codecov / codecov/patch

aiohttp/client_reqrep.py#L1041-L1042 

Added lines #L1041 - L1042 were not covered by tests
self.release()

async def read(self) -> bytes:
Expand Down