Skip to content

[Core] Update aiohttp transport timeout errors #41227

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

Merged
merged 6 commits into from
May 22, 2025
Merged

Conversation

annatisch
Copy link
Member

@annatisch annatisch commented May 20, 2025

Fixes #39448

This PR makes two changes:

  • The ConnectionTimeoutError was added to aiohttp in v3.10, this patch checks for the new error type, and raises a retriable ServiceRequestTimeoutError in such cases.
  • The existing behaviour (which is also used in cases where aiohttp >3.10) has been updated to replace ServiceResponseError with the more specific child class ServiceResponseTimeoutError.

@annatisch annatisch marked this pull request as ready for review May 22, 2025 00:54
Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

We also catch asyncio.TimeoutError in AioHttpStreamDownloadGenerator. Should we apply similar logic there as well?

@annatisch
Copy link
Member Author

annatisch commented May 22, 2025

We also catch asyncio.TimeoutError in AioHttpStreamDownloadGenerator. Should we apply similar logic there as well?

This is a good question - I will double check.
I suspect we may not need to, because this error is raised on the initial connect, which I suspect we would already have successfully completed by the time we actually have a response to stream.

Update
Yeah I don't think we need to add any logic to the stream generators - this is the only place where a ConnectionTimeoutError is raised in aiohttp:
https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L657-L664
This error is not raised if the connection were to then fail during the stream.
I have added a stream=True check to the tests as well.

Copy link
Member

@kushagraThapar kushagraThapar left a comment

Choose a reason for hiding this comment

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

Thanks a lot @annatisch for fixing this, appreciate your work on this!

Copy link
Member

@kristapratico kristapratico left a comment

Choose a reason for hiding this comment

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

Should we add a changelog entry?

@johanste
Copy link
Member

Should we add a changelog entry?

Yes.

@annatisch annatisch merged commit 1163ea4 into main May 22, 2025
50 checks passed
@annatisch annatisch deleted the annatisch-patch-21 branch May 22, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate mapping of aiohttp.ConnectionErrors from asyncio.TimeoutError into ServiceRequestError and ServiceResponseError respectively
5 participants