Ignore protocol exceptions after it is closed

[greshilov]

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]

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

[patchback]

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

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. 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

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

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

5. 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.
6. Push this branch to your fork on GitHub:

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

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

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

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. 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

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

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

5. 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.
6. Push this branch to your fork on GitHub:

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

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

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