Timeout and retry fixes #44
Conversation
|
btw, I'm not sure the retrylogic is completely correct. in botocore.retryhandler it uses an EXCEPTION_MAP which currently has urllib3 exceptions in them: ConnectionError, ClosedPoolError. I think this whole sections needs to be reviewed to make sure the aiohttp exceptions will be handled correctly. |
|
Could you point me where is time.sleep logic? From quick look I do not see it... |
|
Sorry found. |
|
I found it while I was trying to see if I could use the botocore retry logic for aio-s3 :) Found this gem too: aws/aws-cli#1092 we can't programmatically specify the retry count :( |
|
Changes looks good, I will execute test suite and mere this PR tomorrow. This types of bugs is very nasty and hard to find, I should have enabled logging for slow callbacks in event loop: https://docs.python.org/3/library/asyncio-dev.html#debug-mode-of-asyncio long time ago. |
|
cool, what do you think about the issues with the aiohttp exception types? Seems like it will require some research. |
|
I think that logic should be not that bad : https://github.com/boto/botocore/blob/90382a41c5a23e27eda1c8fbf23fc1a3b17804ed/botocore/endpoint.py#L175-L204 here |
|
It could be improved but issue is not the end of the world at least :) |
|
the issue is that it may not retry aiohttp connection exceptions as expected, see https://github.com/boto/botocore/blob/90382a41c5a23e27eda1c8fbf23fc1a3b17804ed/botocore/retryhandler.py#L147 |
|
Oh, I see, that should be investigated, I have created issue #47 in order not to forget. |
|
btw, added some more clean-up and a couple bugfixes. No more unclosed sessions, yay |
|
just submitted fix for #47 |
|
Sorry for delay, I manually merged part of commits from this PR to master. Lets do retry logic in separate PR. I will outline some issue here. |
| self.close() | ||
| # callers may not read the Body resulting in an unclosed ClientResponse | ||
| if self._loop.is_running(): | ||
| asyncio.ensure_future(self.__response.release(), loop=self._loop) |
There was a problem hiding this comment.
If body is not fetched, connection and response should be closed with warning like aiohttp does, you can use:
(self.__response.close() without ensure_future. ensure_future is dangerous here because we can loose exception.
There was a problem hiding this comment.
from what I understand release is better than close as close can close the connector. And close is a coroutine so you need to run it with an ensure_future. Perhaps then just remove the del? that way you get the warning
There was a problem hiding this comment.
So socket connection is not useful if all data from buffer has not been fetched, release() tries to read all available data in order to save connection and return it to connection pool (session). But since this object is garbage collected anyway, we should drop connection and warn user that he should release/close connection explicitly.
There was a problem hiding this comment.
here could be corner case for instance, user decided not to fetch huge file (1GB), and ensure_future with release will try to read all the data on backgound.
There was a problem hiding this comment.
actually ya that's a better example, also I found an issue with the new properties as it seems someone is trying to set them, working on fix
There was a problem hiding this comment.
removed del and added comment about behavior
| @@ -183,6 +186,17 @@ def get_paginator(self, operation_name): | |||
| self._cache['page_config'][actual_operation_name]) | |||
| return paginator | |||
|
|
|||
| if PY_35: | |||
| @asyncio.coroutine | |||
| def __aenter__(self): | |||
There was a problem hiding this comment.
just curious why we need this? Looks like it is not public api
There was a problem hiding this comment.
this is so you can do:
session = aiobotocore.get_session()
async with session.create_client() as client:
# do workthis way the TCPConnector is closed when you're done using the client and you don't need to manually call client.close()
There was a problem hiding this comment.
Look good! we need some tests for that in separate file, because this is only python3.5 ((
|
closing and opening new PR per comments |
Timeouts
Retries
Misc