Fix closing client session by atemate · Pull Request #3733 · aio-libs/aiohttp

atemate · 2019-05-07T19:16:04Z

What do these changes do?

Make BaseConnector.close() an asynchronous function (still BaseConnector._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()
Deprecated with Connector(): syntax is dropped

Related issue number

#3736 #1925.
This PR removes functionality deprecated in PR #3417.

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .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.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

aiohttp/base_protocol.py

aiohttp/connector.py

codecov-io · 2019-05-07T23:22:26Z

Codecov Report

Merging #3733 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@            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

Impacted Files	Coverage Δ
aiohttp/client_proto.py	`96.64% <100%> (+0.09%)`	⬆️
aiohttp/connector.py	`96.72% <100%> (-0.96%)`	⬇️
aiohttp/web_fileresponse.py	`96.55% <0%> (-1.15%)`	⬇️
aiohttp/helpers.py	`97.22% <0%> (-0.26%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcce5ea...fd2f90e. Read the comment docs.

aiohttp/base_protocol.py

aiohttp/connector.py

aiohttp/base_protocol.py

asvetlov · 2019-05-24T14:41:47Z

Please don't forget to update docs.
http://docs.aiohttp.org/en/stable/client_advanced.html and http://docs.aiohttp.org/en/stable/client_reference.html mention connectors. These pages should reflect the PR changes.

atemate · 2019-06-07T16:41:52Z

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

socketpair · 2019-06-28T13:48:58Z

aiohttp/connector.py

+        if waiters:
+            await asyncio.gather(*waiters,
+                                 loop=self._loop,
+                                 return_exceptions=True)


possibly re-raise ? I'm unsure

Ineed, we lost exceptions here. But maybe we should just log (as warnings) them instead?

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.

socketpair · 2019-06-28T13:49:33Z

aiohttp/connector.py

                proto.close()
+                waiters.append(proto.closed)

+            # TODO (A.Yushovskiy, 24-May-2019) collect transp. closing futures


incomplete task ?

work with transports here, especially self._cleanup_closed_transports, seem to be a workaround of the non-closed transports for SSL only:

aiohttp/aiohttp/connector.py

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))

asvetlov

Looks good but please fix my comments.

asvetlov · 2019-07-02T18:17:26Z

aiohttp/connector.py

    from .client_reqrep import ConnectionKey  # noqa
    from .tracing import Trace  # noqa

+_ListOfFutures = List['asyncio.Future[Optional[Exception]]']


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.

asvetlov · 2019-07-02T18:43:09Z

aiohttp/connector.py

+        if waiters:
+            await asyncio.gather(*waiters,
+                                 loop=self._loop,
+                                 return_exceptions=True)


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.

asvetlov · 2019-07-02T18:44:02Z

aiohttp/connector.py

    CeilTimeout,
    get_running_loop,
    is_ip_address,
-    noop2,


Please delete helpers.noop2 function as well.

asvetlov

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

Thie is a partial backport of #3733 without the breaking changes fixes #1925

…ections to close (#11074) fixes #1925 fixes #3736

…ections to close (#11074) fixes #1925 fixes #3736 (cherry picked from commit e550c78)

This is a followup to #11100 and #3733 to avoid creating closed futures that will never be awaited.

atemate changed the title ~~[WIP] Fix closing client session~~ Fix closing client session May 7, 2019

atemate marked this pull request as ready for review May 7, 2019 21:48

atemate requested a review from asvetlov as a code owner May 7, 2019 21:48

atemate commented May 7, 2019

View reviewed changes

aiohttp/base_protocol.py Outdated Show resolved Hide resolved

atemate commented May 7, 2019

View reviewed changes

aiohttp/connector.py Outdated Show resolved Hide resolved

asvetlov requested changes May 13, 2019

View reviewed changes

aiohttp/base_protocol.py Outdated Show resolved Hide resolved

asvetlov requested changes May 24, 2019

View reviewed changes

aiohttp/connector.py Outdated Show resolved Hide resolved

aiohttp/connector.py Outdated Show resolved Hide resolved

aiohttp/base_protocol.py Outdated Show resolved Hide resolved

Artem Yushkovskiy added 15 commits June 28, 2019 14:52

fix closing client session

4e4d043

add changelog

b74542b

fix tests

9748dec

revert noise

53ff05c

review: move closed to ResponseHandler

ca25cef

rename _close()

d1f1f5d

cleanup

62e2d4d

few more comments

13dcd7a

fix TCPConnector._close

ea33a9d

satisfy flake8

2f6fcd7

reduce noise

dc66fc5

fix mypy

88e815b

fix towncrier

a031cd0

cleanup + doc

b37ebad

lint (syntax err)

27484ff

atemate requested a review from webknjaz as a code owner June 28, 2019 13:04

flake

00b5088

socketpair reviewed Jun 28, 2019

View reviewed changes

mypy

a46ee7e

drop unnecessary file

fd2f90e

asvetlov reviewed Jul 2, 2019

View reviewed changes

Artem Yushkovskiy added 2 commits July 3, 2019 17:50

review changes

9472026

fix build for older pythons

b7fedd8

asvetlov approved these changes Jul 4, 2019

View reviewed changes

asvetlov merged commit 6b0bc4e into aio-libs:master Jul 4, 2019

atemate deleted the ay/fix-close-client-session branch July 4, 2019 13:10

blueyed mentioned this pull request Jul 16, 2019

Pass loop to ResponseHandler for aiohttp 4 pnuckowski/aioresponses#129

Merged

keysmashes mentioned this pull request Aug 6, 2020

Deprecation: @coroutine" decorator is deprecated since Python 3.8, use "async def" instead. #4842

Closed

Dreamsorcerer mentioned this pull request Apr 28, 2023

Wait until client closes all connections on BaseConnector.close() #3736

Closed

bdraco added a commit that referenced this pull request May 28, 2025

Wait for connections to close in connector.close()

15695d6

Thie is a partial backport of #3733 without the breaking changes fixes #1925

bdraco mentioned this pull request May 28, 2025

[PR #3733/6b0bc4ef backport][3.13] Fix connector not waiting for connections to close #11074

Merged

5 tasks

bdraco added a commit that referenced this pull request May 29, 2025

[PR #3733/6b0bc4ef backport][3.13] Fix connector not waiting for conn…

e550c78

…ections to close (#11074) fixes #1925 fixes #3736

patchback bot pushed a commit that referenced this pull request May 29, 2025

[PR #3733/6b0bc4ef backport][3.13] Fix connector not waiting for conn…

6c4447d

…ections to close (#11074) fixes #1925 fixes #3736 (cherry picked from commit e550c78)

patchback bot mentioned this pull request May 29, 2025

[PR #11074/e550c78a backport][3.11] Fix connector not waiting for connections to close #11076

Merged

5 tasks

patchback bot pushed a commit that referenced this pull request May 29, 2025

[PR #3733/6b0bc4ef backport][3.13] Fix connector not waiting for conn…

016f478

…ections to close (#11074) fixes #1925 fixes #3736 (cherry picked from commit e550c78)

patchback bot mentioned this pull request May 29, 2025

[PR #11074/e550c78a backport][3.12] Fix connector not waiting for connections to close #11077

Merged

5 tasks

This was referenced May 29, 2025

Not waiting for connection_lost() when transport closed #1925

Closed

Fix spurious "Future exception was never retrieved" warnings for connection lost errors #11100

Merged

bdraco added a commit that referenced this pull request Jun 1, 2025

Avoid creating closed futures that will never be awaited

4002a03

This is a followup to #11100 and #3733 to avoid creating closed futures that will never be awaited.

bdraco mentioned this pull request Jun 1, 2025

Avoid creating closed futures that will never be awaited #11107

Merged

bdraco added a commit that referenced this pull request Jun 1, 2025

Avoid creating closed futures that will never be awaited

f720eb9

This is a followup to #11100 and #3733 to avoid creating closed futures that will never be awaited.

bdraco mentioned this pull request Jun 2, 2025

Downgrade connector close error to debug #11114

Merged

mifuyutsuki mentioned this pull request Aug 11, 2025

[BUG] RuntimeError after bot termination with aiohttp>=3.12.4 interactions-py/interactions.py#1778

Closed

4 tasks

	if (key.is_ssl and
	not self._cleanup_closed_disabled):
	self._cleanup_closed_transports.append(
	transport)

Uh oh!

Conversation

atemate commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

Uh oh!

Uh oh!

codecov-io commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asvetlov commented May 24, 2019

Uh oh!

atemate commented Jun 7, 2019

Uh oh!

socketpair Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

atemate commented May 7, 2019 •

edited

Loading

codecov-io commented May 7, 2019 •

edited

Loading

socketpair Jun 28, 2019 •

edited

Loading