Borrow asyncio ssl implementation from uvloop #88177

asvetlov · 2021-05-02T20:33:40Z

BPO	44011
Nosy	@asvetlov, @1st1, @pablogsal, @kumaraditya303
PRs	bpo-44011: New asyncio ssl implementation #17975 bpo-44011: Fix asyncio tests without ssl module (GH-25840) #25840 bpo-44011: Increase test timeout #25842 bpo-44011: Increase timeouts for asyncio SSL tests #25846 bpo-44011: Revert "New asyncio ssl implementation (GH-17975)" #25848 bpo-44011: New asyncio ssl implementation #31275 bpo-44011: add docs for ssl_shutdown_timeout parameter #31597

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/asvetlov'
closed_at = <Date 2022-02-22.21:12:45.984>
created_at = <Date 2021-05-02.20:33:39.996>
labels = ['3.11', 'type-feature', 'expert-asyncio']
title = 'Borrow asyncio ssl implementation from uvloop'
updated_at = <Date 2022-02-26.13:06:50.530>
user = 'https://github.com/asvetlov'

bugs.python.org fields:

activity = <Date 2022-02-26.13:06:50.530>
actor = 'asvetlov'
assignee = 'asvetlov'
closed = True
closed_date = <Date 2022-02-22.21:12:45.984>
closer = 'asvetlov'
components = ['asyncio']
creation = <Date 2021-05-02.20:33:39.996>
creator = 'asvetlov'
dependencies = []
files = []
hgrepos = []
issue_num = 44011
keywords = ['patch']
message_count = 18.0
messages = ['392726', '392739', '392769', '392771', '392774', '392775', '392776', '392798', '392799', '392800', '392802', '392813', '392815', '413062', '413065', '413288', '413289', '414102']
nosy_count = 4.0
nosy_names = ['asvetlov', 'yselivanov', 'pablogsal', 'kumaraditya']
pr_nums = ['17975', '25840', '25842', '25846', '25848', '31275', '31597']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue44011'
versions = ['Python 3.11']

asvetlov · 2021-05-02T20:33:40Z

There is a PR created a long time ago.
Finally, I've ported tests for it also.

The documentation doesn't mention new ssh_shutdown_timeout parameter yet.

The latest changes from MagicStack/uvloop#385 can be applied separately.

asvetlov · 2021-05-02T21:34:22Z

New changeset 5fb06ed by Andrew Svetlov in branch 'master':
bpo-44011: New asyncio ssl implementation (bpo-17975)
5fb06ed

tiran · 2021-05-03T06:32:51Z

The commit has broken multiple build bots, e.g .https://buildbot.python.org/all/#/builders/345/builds/134/steps/5/logs/stdio The new code is missing checks for presence of ssl module. It's an optional component.

tirkarthi · 2021-05-03T07:16:17Z

The PR made sslproto a hard dependency that even import asyncio fails on non-ssl builds and thus anything that indirectly import asyncio also fails. It seems some of the test modules can be skipped. Some parts of the asyncio codebase already has checks for ssl and has to be done for new parts. Attached is a patch to add more checks but it will be helpful to ensure only relevant parts that absolutely require ssl are skipped.

The test_make_socket_transport is slightly tricky since it tries to simulate ssl being not present by patching it but mock does import of sslproto which will fail since SSLAgainErrors is initialized at module level. Perhaps the test can be modified better to only mock if ssl is not present.

diff --git a/Lib/asyncio/base_events.py b/Lib/asyncio/base_events.py
index e54ee309e4..6ccac76dfb 100644
--- a/Lib/asyncio/base_events.py
+++ b/Lib/asyncio/base_events.py
@@ -41,13 +41,14 @@
 from . import exceptions
 from . import futures
 from . import protocols
-from . import sslproto
 from . import staggered
 from . import tasks
 from . import transports
 from . import trsock
 from .log import logger
 
+if ssl is not None:
+    from . import sslproto
 
 __all__ = 'BaseEventLoop',
 
diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py
index 10852afe2b..ac0dc1978c 100644
--- a/Lib/asyncio/proactor_events.py
+++ b/Lib/asyncio/proactor_events.py
@@ -19,11 +19,17 @@
 from . import futures
 from . import exceptions
 from . import protocols
-from . import sslproto
 from . import transports
 from . import trsock
 from .log import logger
 
+try:
+    import ssl
+except ImportError:  # pragma: no cover
+    ssl = None
+
+if ssl is not None:
+    from . import sslproto
 
 def _set_socket_extra(transport, sock):
     transport._extra['socket'] = trsock.TransportSocket(sock)
@@ -826,6 +832,9 @@ def loop(f=None):
                                      server, addr, conn)
                     protocol = protocol_factory()
                     if sslcontext is not None:
+                        if ssl is None:
+                            raise RuntimeError('Python ssl module is not available')
+
                         self._make_ssl_transport(
                             conn, protocol, sslcontext, server_side=True,
                             extra={'peername': addr}, server=server,
diff --git a/Lib/asyncio/selector_events.py b/Lib/asyncio/selector_events.py
index 63ab15f30f..9bc9a03699 100644
--- a/Lib/asyncio/selector_events.py
+++ b/Lib/asyncio/selector_events.py
@@ -23,11 +23,12 @@
 from . import events
 from . import futures
 from . import protocols
-from . import sslproto
 from . import transports
 from . import trsock
 from .log import logger
 
+if ssl is not None:
+    from . import sslproto
 
 def _test_selector_event(selector, fd, event):
     # Test if the selector is monitoring 'event' events
@@ -213,6 +214,9 @@ def _accept_connection(
             protocol = protocol_factory()
             waiter = self.create_future()
             if sslcontext:
+                if ssl is None:
+                    raise RuntimeError('Python ssl module is not available')
+
                 transport = self._make_ssl_transport(
                     conn, protocol, sslcontext, waiter=waiter,
                     server_side=True, extra=extra, server=server,
diff --git a/Lib/test/test_asyncio/test_selector_events.py b/Lib/test/test_asyncio/test_selector_events.py
index 349e4f2dca..6aaa7a86be 100644
--- a/Lib/test/test_asyncio/test_selector_events.py
+++ b/Lib/test/test_asyncio/test_selector_events.py
@@ -70,6 +70,7 @@ def test_make_socket_transport(self):
 
         close_transport(transport)
 
+    @unittest.skipIf(ssl is None, 'No ssl module')
     @mock.patch('asyncio.selector_events.ssl', None)
     @mock.patch('asyncio.sslproto.ssl', None)
     def test_make_ssl_transport_without_ssl_error(self):
diff --git a/Lib/test/test_asyncio/test_ssl.py b/Lib/test/test_asyncio/test_ssl.py
index 38235c63e0..c58346bcab 100644
--- a/Lib/test/test_asyncio/test_ssl.py
+++ b/Lib/test/test_asyncio/test_ssl.py
@@ -1,3 +1,8 @@
+from test.support import import_helper
+
+# Skip tests if we don't have ssl
+import_helper.import_module('ssl')
+
 import asyncio
 import asyncio.sslproto
 import contextlib
diff --git a/Lib/test/test_asyncio/test_sslproto.py b/Lib/test/test_asyncio/test_sslproto.py
index 79a81bd8c3..2edbb11b58 100644
--- a/Lib/test/test_asyncio/test_sslproto.py
+++ b/Lib/test/test_asyncio/test_sslproto.py
@@ -11,6 +11,9 @@
 except ImportError:
     ssl = None
 
+# Skip tests if we don't have ssl
+support.import_helper.import_module('ssl')
+
 import asyncio
 from asyncio import log
 from asyncio import protocols

tiran · 2021-05-03T07:32:22Z

PR #70027 fixes most issues. Gentoo with X buildbot https://buildbot.python.org/all/#builders/465/builds/23 has one failing test.

======================================================================
ERROR: test_create_server_ssl_over_ssl (test.test_asyncio.test_ssl.TestSSL)
----------------------------------------------------------------------

asyncio.exceptions.CancelledError
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/pull_request.ware-gentoo-x86.installed/build/target/lib/python3.10/asyncio/tasks.py", line 458, in wait_for
    fut.result()
asyncio.exceptions.CancelledError
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/buildbot/buildarea/cpython/pull_request.ware-gentoo-x86.installed/build/target/lib/python3.10/test/test_asyncio/test_ssl.py", line 1157, in test_create_server_ssl_over_ssl
    self.loop.run_until_complete(start_server())
  File "/buildbot/buildarea/cpython/pull_request.ware-gentoo-x86.installed/build/target/lib/python3.10/asyncio/base_events.py", line 644, in run_until_complete
    return future.result()
  File "/buildbot/buildarea/cpython/pull_request.ware-gentoo-x86.installed/build/target/lib/python3.10/test/test_asyncio/test_ssl.py", line 1150, in start_server
    await asyncio.wait_for(asyncio.gather(*tasks), TIMEOUT)
  File "/buildbot/buildarea/cpython/pull_request.ware-gentoo-x86.installed/build/target/lib/python3.10/asyncio/tasks.py", line 460, in wait_for
    raise exceptions.TimeoutError() from exc
asyncio.exceptions.TimeoutError

tiran · 2021-05-03T07:39:06Z

New changeset 37ebdf0 by Christian Heimes in branch 'master':
bpo-44011: Fix asyncio tests without ssl module (GH-25840)
37ebdf0

tiran · 2021-05-03T07:41:57Z

I have merged my PR to unblock buildbots. Karthikeyan has made suggestions how to improve the tests even further. CI also had some issues with OpenSSL 3.0.0-alpha15. Please run the tests with new OpenSSL version, too. "make multissltest" automates download, compilation, local installation, and testing.

pablogsal · 2021-05-03T12:34:46Z

Since commit 5fb06ed was merged there are multiple timeouts in several buildbots. Unfortunately if this is not fixed by the time I need to do the beta release I may need to revert all these commits until all buildbots are stable again.

Could someone investigate those timeouts?

For instance, check:

https://buildbot.python.org/all/#/builders/464/builds/138

tiran · 2021-05-03T12:41:11Z

When was https://buildbot.python.org/all/#/builders/464/builds/138 start? The build properties tab doesn't have a start timestamp.

Andrew, increase timeout doesn't seem to help. It's looks like the test suite is leaking threads on error.

pablogsal · 2021-05-03T12:56:11Z

I have created #25848 for the revert, in case this is not fixed in the next hours or so.

Given the nature of PR 17975, this should have tested with the buildbots as the release team asked in:

https://mail.python.org/archives/list/python-committers@python.org/thread/SIJQE3BZ6ICCGNJWFR4YR65BQBJJZZAZ/

and

https://mail.python.org/archives/list/python-committers@python.org/thread/V7V5JHKZCJVE2GTI5NFEP3PNKOLH35VL/

pablogsal · 2021-05-03T13:00:17Z

Specifically this part of both messages:

> If your change involves some platform-specific behaviour or has a
> non-trivial amount of C code, make sure you run the buildbots
> in your Pull Request by using the "test-with-buildbots" label (
> https://discuss.python.org/t/now-you-can-test-a-pr-with-the-buildbots-before...).
> Alternatively you could check the buildbots post-merge in the buildbot server:
> https://buildbot.python.org/all/#/builders?tags=%2B3.x&tags=%2Bstable
> This is very important because if problems are detected at the time >> of the
> release, the release management team may have to revert
> the changes and therefore those will not be included in Python 3.10.

pablogsal · 2021-05-03T15:22:03Z

New changeset 7719953 by Pablo Galindo in branch 'master':
bpo-44011: Revert "New asyncio ssl implementation (GH-17975)" (GH-25848)
7719953

pablogsal · 2021-05-03T15:37:22Z

Unfortunately I have reverted 5fb06ed commit to unblock the beta release :(

I know that nobody wants this but my responsibilities as a release manager is to safeguard the stability of the release and we are too close to the beta release to do all the testing we need, giving that many buildbots have been broken in a short timespan.

Andrew, we can try to get your PR merge between beta 1 and beta 2 but once we have done extensive testing and we know that there will be no impact on the buildbots and the CI.

Thank you all for your understanding

kumaraditya303 · 2022-02-11T12:13:57Z

Since it was reverted as it was beta period, Can this be committed again as 3.11 is in alpha currently? @asvetlov

kumaraditya303 · 2022-02-11T12:23:50Z

I created a draft PR by rebasing the old implementation of 3.10 for 3.11 so we can investigate the build-bots failure and fix them so this can be committed for 3.11.

See #31275

asvetlov · 2022-02-15T13:04:09Z

New changeset 13c10bf by Kumar Aditya in branch 'main':
bpo-44011: New asyncio ssl implementation (bpo-31275)
13c10bf

asvetlov · 2022-02-15T13:05:59Z

The code had landed.
Need a follow-up PR with documentation update (mention new ssl_shutdown_timeout arguments)

asvetlov · 2022-02-26T13:06:50Z

New changeset 41ddcd3 by Kumar Aditya in branch 'main':
bpo-44011: Document ssl_shutdown_timeout added by revisited asyncio SSL implementation (GH-31597)
41ddcd3

asvetlov added 3.10 only security fixes topic-asyncio labels May 2, 2021

tiran added the release-blocker label May 3, 2021

tiran added type-feature A feature request or enhancement labels May 3, 2021

tiran removed the release-blocker label May 3, 2021

tiran assigned asvetlov May 3, 2021

tiran removed the release-blocker label May 3, 2021

tiran assigned asvetlov May 3, 2021

kumaraditya303 added 3.11 only security fixes and removed 3.10 only security fixes labels Feb 11, 2022

asvetlov closed this as completed Feb 22, 2022

ezio-melotti transferred this issue from another repository Apr 10, 2022

vstinner mentioned this issue Aug 4, 2022

test_asyncio: test_create_server_ssl_over_ssl() failed on ARM64 macOS 3.x #95573

Closed

tiran mentioned this issue Aug 4, 2022

New asyncio ssl implementation is lacking license and origin information #95649

Closed

bedevere-bot mentioned this issue Aug 4, 2022

[3.11] gh-88177: Revert the new asyncio.sslproto implementation #95663

Closed

This was referenced Sep 15, 2022

_SSLProtocolTransport.close() cannot close the connection until timeout or EOF MagicStack/uvloop#471

Open

(client) connections cannot be really closed before EOF with uvloop enabled aio-libs/aiohttp#6762

Closed

skshetry mentioned this issue Jan 5, 2023

_prepare_credentials: connector fix for fsspec>=2022.12 iterative/dvc-http#28

Merged

vEpiphyte mentioned this issue Mar 20, 2023

Locking issue with asyncio StreamWriter.drain() with SSL Transport #102792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Borrow asyncio ssl implementation from uvloop #88177

Borrow asyncio ssl implementation from uvloop #88177

asvetlov commented May 2, 2021

asvetlov commented May 2, 2021

asvetlov commented May 2, 2021

tiran commented May 3, 2021

tirkarthi commented May 3, 2021

tiran commented May 3, 2021

tiran commented May 3, 2021

tiran commented May 3, 2021

pablogsal commented May 3, 2021

tiran commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

kumaraditya303 commented Feb 11, 2022

kumaraditya303 commented Feb 11, 2022

asvetlov commented Feb 15, 2022

asvetlov commented Feb 15, 2022

asvetlov commented Feb 26, 2022

Borrow asyncio ssl implementation from uvloop #88177

Borrow asyncio ssl implementation from uvloop #88177

Comments

asvetlov commented May 2, 2021

asvetlov commented May 2, 2021

asvetlov commented May 2, 2021

tiran commented May 3, 2021

tirkarthi commented May 3, 2021

tiran commented May 3, 2021

tiran commented May 3, 2021

tiran commented May 3, 2021

pablogsal commented May 3, 2021

tiran commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

pablogsal commented May 3, 2021

kumaraditya303 commented Feb 11, 2022

kumaraditya303 commented Feb 11, 2022

asvetlov commented Feb 15, 2022

asvetlov commented Feb 15, 2022

asvetlov commented Feb 26, 2022