Ignore protocol exceptions after it is closed by greshilov · Pull Request #6321 · aio-libs/aiohttp

greshilov · 2021-11-17T17:32:47Z

Issue

I've recently started to experience strange ssl exceptions with aiohttp 4+ (master branch) with the following message:

Future exception was never retrieved
future: <Future finished exception=SSLError(1, '[SSL: APPLICATION_DATA_AFTER_CLOSE_NOTIFY] application data after close notify (_ssl.c:2756)') created at /usr/lib/python3.9/asyncio/base_events.py:424>
source_traceback: 
 ...
  File "/usr/lib/python3.9/asyncio/base_events.py", line 1081, in create_connection
    transport, protocol = await self._create_connection_transport(
  File "/usr/lib/python3.9/asyncio/base_events.py", line 1099, in _create_connection_transport
    protocol = protocol_factory()
  File "/home/s/python/aiohttp/aiohttp/client_proto.py", line 39, in __init__
    self.closed = self._loop.create_future()  # type: asyncio.Future[None]
  File "/usr/lib/python3.9/asyncio/base_events.py", line 424, in create_future
    return futures.Future(loop=self)
Traceback (most recent call last):
  File "/usr/lib/python3.9/asyncio/sslproto.py", line 528, in data_received
    ssldata, appdata = self._sslpipe.feed_ssldata(data)
  File "/usr/lib/python3.9/asyncio/sslproto.py", line 206, in feed_ssldata
    self._sslobj.unwrap()
  File "/usr/lib/python3.9/ssl.py", line 948, in unwrap
    return self._sslobj.shutdown()
ssl.SSLError: [SSL: APPLICATION_DATA_AFTER_CLOSE_NOTIFY] application data after close notify (_ssl.c:2756

Reproducer

Code to reproduce the error.

import asyncio
import aiohttp

async def main():

    async with aiohttp.ClientSession() as session:
        async with session.get(f'http://drive.google.com') as resp:
            print(resp.status)

if __name__ == '__main__':
    asyncio.run(main(), debug=True)

Description

After some digging I found out a related issue in python bug tracker:
https://bugs.python.org/issue39951

It describes possible scenario for this type of SSL exception to happen:

client closes the ssl connection and sends close notify.
however server sends some data before receiving that close notify.
client receives the data, but the connection is already closed, thus SSL exception.

In aiohttp this exception happens when BaseConnector._release is called with should_close=True

aiohttp/aiohttp/connector.py

Lines 628 to 633 in d149eff

    
           if should_close or protocol.should_close: 
        
               transport = protocol.transport 
        
               protocol.close() 
        
               if key.is_ssl and not self._cleanup_closed_disabled: 
        
                   self._cleanup_closed_transports.append(transport)

After execution of this branch protocol is on its way to be removed by GC. But if the protocol.connection_lost is called with exception before the removal (which is exactly what is happening) the protocol.closed future will store this exception.

aiohttp/aiohttp/client_proto.py

Lines 73 to 79 in d149eff

    
           def connection_lost(self, exc: Optional[BaseException]) -> None: 
        
               self._drop_timeout() 
        
               if exc is not None: 
        
                   set_exception(self.closed, exc) 
        
               else: 
        
                   set_result(self.closed, None)

During the removal of the protocol object, the following exception will be produced: Future exception was never retrieved.

Solution

I solved it by setting the result of self.closed to None explicitly during the _release call with should_close=True.

Related issue number

Fixes #4526

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."

codecov · 2021-11-17T20:33:55Z

Codecov Report

Merging #6321 (be01461) into master (d149eff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #6321   +/-   ##
=======================================
  Coverage   93.31%   93.32%           
=======================================
  Files         103      103           
  Lines       30369    30374    +5     
  Branches     2731     2730    -1     
=======================================
+ Hits        28340    28347    +7     
+ Misses       1852     1850    -2     
  Partials      177      177

Flag	Coverage Δ
unit	`93.25% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/connector.py	`93.93% <100.00%> (+<0.01%)`	⬆️
tests/test_connector.py	`95.72% <100.00%> (+0.01%)`	⬆️
aiohttp/web_request.py	`95.90% <0.00%> (ø)`
aiohttp/helpers.py	`96.93% <0.00%> (+<0.01%)`	⬆️
aiohttp/client.py	`24.67% <0.00%> (+0.10%)`	⬆️

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 d149eff...be01461. Read the comment docs.

Dreamsorcerer

Confirmed that this fixes #4526

Co-authored-by: Sam Bull <[email protected]>

patchback · 2022-01-19T15:25:48Z

Backport to 3.8: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 58da337 on top of patchback/backports/3.8/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321

Backporting merged PR #6321 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.8/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321 upstream/3.8

Now, cherry-pick PR Ignore protocol exceptions after it is closed #6321 contents into that branch:
```
$ git cherry-pick -x 58da3373a21211bbde21f1393475e73f53a5671d
```
If it'll yell at you with something like fatal: Commit 58da3373a21211bbde21f1393475e73f53a5671d is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 58da3373a21211bbde21f1393475e73f53a5671d
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Ignore protocol exceptions after it is closed #6321 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.8/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback · 2022-01-19T15:25:58Z

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 58da337 on top of patchback/backports/3.9/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321

Backporting merged PR #6321 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.9/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321 upstream/3.9

Now, cherry-pick PR Ignore protocol exceptions after it is closed #6321 contents into that branch:
```
$ git cherry-pick -x 58da3373a21211bbde21f1393475e73f53a5671d
```
If it'll yell at you with something like fatal: Commit 58da3373a21211bbde21f1393475e73f53a5671d is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 58da3373a21211bbde21f1393475e73f53a5671d
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Ignore protocol exceptions after it is closed #6321 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.9/58da3373a21211bbde21f1393475e73f53a5671d/pr-6321

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

asvetlov · 2022-01-19T15:38:09Z

Backporting doesn't make sense, protocol has no closed property

greshilov requested a review from asvetlov as a code owner November 17, 2021 17:32

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Nov 17, 2021

greshilov force-pushed the pr-4526 branch 2 times, most recently from 155744c to b046b2c Compare November 17, 2021 17:40

greshilov changed the title ~~Ignore protocol exceptions after it is closed~~ [WIP] Ignore protocol exceptions after it is closed Nov 17, 2021

greshilov force-pushed the pr-4526 branch from b046b2c to fdc2872 Compare November 17, 2021 20:23

greshilov changed the title ~~[WIP] Ignore protocol exceptions after it is closed~~ Ignore protocol exceptions after it is closed Nov 17, 2021

Ignore protocol exceptions after it is closed

398f4d5

greshilov force-pushed the pr-4526 branch from fdc2872 to 398f4d5 Compare November 17, 2021 20:49

greshilov mentioned this pull request Nov 18, 2021

"application data after close notify on" 4.0.0a1 #4526

Closed

Dreamsorcerer reviewed Nov 19, 2021

View reviewed changes

Comment thread aiohttp/connector.py Outdated

Dreamsorcerer approved these changes Nov 19, 2021

View reviewed changes

Add TODO

be01461

Co-authored-by: Sam Bull <[email protected]>

asvetlov approved these changes Jan 19, 2022

View reviewed changes

asvetlov added backport-3.8 labels Jan 19, 2022

asvetlov merged commit 58da337 into aio-libs:master Jan 19, 2022

bdraco mentioned this pull request Nov 11, 2024

Remove connector close workaround for older Python versions #9818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ignore protocol exceptions after it is closed#6321

Ignore protocol exceptions after it is closed#6321
asvetlov merged 2 commits intoaio-libs:masterfrom
greshilov:pr-4526

greshilov commented Nov 17, 2021 •

edited by Dreamsorcerer

Loading

Uh oh!

codecov bot commented Nov 17, 2021 •

edited

Loading

Uh oh!

Uh oh!

Dreamsorcerer left a comment

Uh oh!

patchback bot commented Jan 19, 2022 •

edited

Loading

Uh oh!

patchback bot commented Jan 19, 2022 •

edited

Loading

Uh oh!

asvetlov commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if should_close or protocol.should_close:
	transport = protocol.transport
	protocol.close()

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

	def connection_lost(self, exc: Optional[BaseException]) -> None:
	self._drop_timeout()

	if exc is not None:
	set_exception(self.closed, exc)
	else:
	set_result(self.closed, None)

Uh oh!

Conversation

greshilov commented Nov 17, 2021 • edited by Dreamsorcerer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Reproducer

Description

Solution

Related issue number

Checklist

Uh oh!

codecov bot commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Dreamsorcerer left a comment

Choose a reason for hiding this comment

Uh oh!

patchback bot commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.8: 💔 cherry-picking failed — conflicts found

Backporting merged PR #6321 into master

Uh oh!

patchback bot commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Backport to 3.9: 💔 cherry-picking failed — conflicts found

Backporting merged PR #6321 into master

Uh oh!

asvetlov commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greshilov commented Nov 17, 2021 •

edited by Dreamsorcerer

Loading

codecov bot commented Nov 17, 2021 •

edited

Loading

patchback bot commented Jan 19, 2022 •

edited

Loading

patchback bot commented Jan 19, 2022 •

edited

Loading