-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix closing client session #3733
Fix closing client session #3733
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3733 +/- ##
==========================================
- Coverage 97.96% 97.86% -0.11%
==========================================
Files 43 43
Lines 8590 8602 +12
Branches 1373 1375 +2
==========================================
+ Hits 8415 8418 +3
- Misses 71 79 +8
- Partials 104 105 +1
Continue to review full report at Codecov.
|
Please don't forget to update docs. |
Sorry for this long-pending PR, I will return back to it in the end of June 2019 when I have more time, thank you |
aiohttp/connector.py
Outdated
if waiters: | ||
await asyncio.gather(*waiters, | ||
loop=self._loop, | ||
return_exceptions=True) |
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.
possibly re-raise ? I'm unsure
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.
Ineed, we lost exceptions here. But maybe we should just log (as warnings) them instead?
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.
Heh, very interesting question.
Python and asyncio itself have no MultipleException yet.
Maybe we add it in Python 3.9.
I'm not sure if warnings.warn()
is a good choice here: it is not a programming error but network issue.
Please use logging.error()
instead.
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.
ok
|
||
# TODO (A.Yushovskiy, 24-May-2019) collect transp. closing futures |
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.
incomplete task ?
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.
work with transports here, especially self._cleanup_closed_transports
, seem to be a workaround of the non-closed transports for SSL only:
Lines 345 to 348 in dcce5ea
if (key.is_ssl and | |
not self._cleanup_closed_disabled): | |
self._cleanup_closed_transports.append( | |
transport) |
Also, transports in _cleanup_closed_transports
are of type asyncio.Transport
so we can not modify them to save closing future same way as we did in this PR with ResponseHandler
, thus I don't see a way to get the awaitable result of transport.abort()
.
Good news is that most likely these calls are redundant since we close all protocols (i.e., save them to the list of futures: waiters.append(proto.closed)
)
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.
Looks good but please fix my comments.
aiohttp/connector.py
Outdated
@@ -72,6 +71,8 @@ | |||
from .client_reqrep import ConnectionKey # noqa | |||
from .tracing import Trace # noqa | |||
|
|||
_ListOfFutures = List['asyncio.Future[Optional[Exception]]'] |
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.
Is it a list of futures that contains None or Exception as a value or list of futures with fut.set_result(None)
or fut.set_exception(exc)
?
If later -- please use Future[None]
.
I believe we never return an exception but set it explicitly.
aiohttp/connector.py
Outdated
if waiters: | ||
await asyncio.gather(*waiters, | ||
loop=self._loop, | ||
return_exceptions=True) |
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.
Heh, very interesting question.
Python and asyncio itself have no MultipleException yet.
Maybe we add it in Python 3.9.
I'm not sure if warnings.warn()
is a good choice here: it is not a programming error but network issue.
Please use logging.error()
instead.
@@ -49,7 +49,6 @@ | |||
CeilTimeout, | |||
get_running_loop, | |||
is_ip_address, | |||
noop2, |
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 delete helpers.noop2
function as well.
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.
Looks good!
This is required since aio-libs/aiohttp@6b0bc4e. Fixes: tests/test_aioresponses.py:266: in test_address_as_instance_of_url_combined_with_pass_through api, ext = yield from doit() tests/test_aioresponses.py:258: in doit api_resp = yield from self.session.get(self.url) aioresponses/core.py:328: in _request_mock response = await self.match(method, url, **kwargs) aioresponses/core.py:304: in match response = await matcher.build_response(url, **kwargs) aioresponses/core.py:184: in build_response reason=result.reason) aioresponses/core.py:157: in _build_response resp.content = stream_reader_factory() aioresponses/compat.py:26: in stream_reader_factory protocol = ResponseHandler(loop=loop) ../aiohttp/aiohttp/client_proto.py:41: in __init__ self.closed = self._loop.create_future() # type: asyncio.Future[None] E AttributeError: 'NoneType' object has no attribute 'create_future' Ref: aio-libs/aiohttp#3733
This is required since aio-libs/aiohttp@6b0bc4e. Fixes: tests/test_aioresponses.py:266: in test_address_as_instance_of_url_combined_with_pass_through api, ext = yield from doit() tests/test_aioresponses.py:258: in doit api_resp = yield from self.session.get(self.url) aioresponses/core.py:328: in _request_mock response = await self.match(method, url, **kwargs) aioresponses/core.py:304: in match response = await matcher.build_response(url, **kwargs) aioresponses/core.py:184: in build_response reason=result.reason) aioresponses/core.py:157: in _build_response resp.content = stream_reader_factory() aioresponses/compat.py:26: in stream_reader_factory protocol = ResponseHandler(loop=loop) ../aiohttp/aiohttp/client_proto.py:41: in __init__ self.closed = self._loop.create_future() # type: asyncio.Future[None] E AttributeError: 'NoneType' object has no attribute 'create_future' Ref: aio-libs/aiohttp#3733
This is required since aio-libs/aiohttp@6b0bc4e. Fixes: tests/test_aioresponses.py:266: in test_address_as_instance_of_url_combined_with_pass_through api, ext = yield from doit() tests/test_aioresponses.py:258: in doit api_resp = yield from self.session.get(self.url) aioresponses/core.py:328: in _request_mock response = await self.match(method, url, **kwargs) aioresponses/core.py:304: in match response = await matcher.build_response(url, **kwargs) aioresponses/core.py:184: in build_response reason=result.reason) aioresponses/core.py:157: in _build_response resp.content = stream_reader_factory() aioresponses/compat.py:26: in stream_reader_factory protocol = ResponseHandler(loop=loop) ../aiohttp/aiohttp/client_proto.py:41: in __init__ self.closed = self._loop.create_future() # type: asyncio.Future[None] E AttributeError: 'NoneType' object has no attribute 'create_future' Ref: aio-libs/aiohttp#3733
This is required since aio-libs/aiohttp@6b0bc4e. Fixes: tests/test_aioresponses.py:266: in test_address_as_instance_of_url_combined_with_pass_through api, ext = yield from doit() tests/test_aioresponses.py:258: in doit api_resp = yield from self.session.get(self.url) aioresponses/core.py:328: in _request_mock response = await self.match(method, url, **kwargs) aioresponses/core.py:304: in match response = await matcher.build_response(url, **kwargs) aioresponses/core.py:184: in build_response reason=result.reason) aioresponses/core.py:157: in _build_response resp.content = stream_reader_factory() aioresponses/compat.py:26: in stream_reader_factory protocol = ResponseHandler(loop=loop) ../aiohttp/aiohttp/client_proto.py:41: in __init__ self.closed = self._loop.create_future() # type: asyncio.Future[None] E AttributeError: 'NoneType' object has no attribute 'create_future' Ref: aio-libs/aiohttp#3733
What do these changes do?
Make
BaseConnector.close()
an asynchronous function (stillBaseConnector._close()
is synchronous and preserves the old behaviour).Now,
await connector.close()
waits until all the connections are closed instead of just sending the closing signal to all the connections. The "Unclosed connection" warnings appeared because some connections required more time to be closed because some protocols (TLS) perform additional steps on connection-close and thus require longer time.Are there changes in behavior for the user?
Yes:
BaseConnector.close()
->await BaseConnector.close()
with Connector():
syntax is droppedRelated issue number
#3736 #1925.
This PR removes functionality deprecated in PR #3417.
Checklist
CONTRIBUTORS.txt
CHANGES
folder<issue_id>.<type>
for example (588.bugfix)issue_id
change it to the pr id after creating the pr.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.