-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
pass timeout to request, so it is propagated into StreamRead and affects .read method #86
Conversation
looking through aiohttp this does seem to be the correct fix for read_timeout. Note: this ends up going to aiohttp.streams.StreamReader. Normally we'd want a unit test however to test this one would have to mock aiohttp and that infrastructure doesn't seem available yet in the existing aiobotocore tests. Asking @jettify to give final approval. |
btw, I think with this the asyncio.wait_for can be removed right? It should now be handled by the TCPConnector's conn_timeout (already set), and now the read_timeout set by this PR. |
btw, per my research mentioned in #88 the removal of the wait_for is indeed correct iff read_timeout is >= conn_timeout (default is 60s each). Could you please add an assert/warning and comment to ensure this is the case in the future...then I'll merge this given jettify hasn't responded yet. |
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.
Please add assert + comment or warning to ensure read_timeout >= conn_timeout since aiohttp adds 'timeout' (now read_timeout) wrapper around the TCPConnector connect. FYI I've created a PR for aiohttp to later be able to clearly set the read + connection timeouts: aio-libs/aiohttp#1523
I've gone ahead and added the warning to your branch. |
- add changes missing from https://github.com/aio-libs/aiobotocore/pull/74/files - fix url due to use of yarl by the latest aiohttp
after running unittests I found that aiobotocore was broken for two reasons:
both have been fixed in this PR along with bumping up the aiohttp requirement |
Currently StreamReader returned by aiohttp is not affected by read timeout passed into client, so .read method has 300 seconds timeout by default, we should pass the read timeout into request so it is set on StreamReader