[PR #8672/c3219bf backport][3.10] Fix TCPConnector doing blocking I/O…

… in the event loop to create the SSLContext (#8673)

Co-authored-by: Sam Bull <[email protected]>
Co-authored-by: pre-commit-ci[bot]

CHANGES/8672.bugfix.rst

@@ -0,0 +1,3 @@
+Fixed :py:class:`aiohttp.TCPConnector` doing blocking I/O in the event loop to create the ``SSLContext`` -- by :user:`bdraco`.
+
+The blocking I/O would only happen once per verify mode. However, it could cause the event loop to block for a long time if the ``SSLContext`` creation is slow, which is more likely during startup when the disk cache is not yet present.

aiohttp/connector.py

@@ -50,7 +50,14 @@
 )
 from .client_proto import ResponseHandler
 from .client_reqrep import ClientRequest, Fingerprint, _merge_ssl_params
-from .helpers import ceil_timeout, is_ip_address, noop, sentinel
+from .helpers import (
+ ceil_timeout,
+ is_ip_address,
+ noop,
+ sentinel,
+ set_exception,
+ set_result,
+)
 from .locks import EventResultOrError
 from .resolver import DefaultResolver
 
@@ -771,6 +778,7 @@ class TCPConnector(BaseConnector):
  """
 
  allowed_protocol_schema_set = HIGH_LEVEL_SCHEMA_SET | frozenset({"tcp"})
+ _made_ssl_context: Dict[bool, "asyncio.Future[SSLContext]"] = {}
 
  def __init__(
  self,
@@ -969,29 +977,24 @@ async def _create_connection(
  return proto
 
  @staticmethod
- @functools.lru_cache(None)
  def _make_ssl_context(verified: bool) -> SSLContext:
+ """Create SSL context.
+
+ This method is not async-friendly and should be called from a thread
+ because it will load certificates from disk and do other blocking I/O.
+ """
  if verified:
  return ssl.create_default_context()
- else:
- sslcontext = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
- sslcontext.options |= ssl.OP_NO_SSLv2
- sslcontext.options |= ssl.OP_NO_SSLv3
- sslcontext.check_hostname = False
- sslcontext.verify_mode = ssl.CERT_NONE
- try:
- sslcontext.options |= ssl.OP_NO_COMPRESSION
- except AttributeError as attr_err:
- warnings.warn(
- "{!s}: The Python interpreter is compiled "
- "against OpenSSL < 1.0.0. Ref: "
- "https://docs.python.org/3/library/ssl.html"
- "#ssl.OP_NO_COMPRESSION".format(attr_err),
- )
- sslcontext.set_default_verify_paths()
- return sslcontext
-
- def _get_ssl_context(self, req: ClientRequest) -> Optional[SSLContext]:
+ sslcontext = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
+ sslcontext.options |= ssl.OP_NO_SSLv2
+ sslcontext.options |= ssl.OP_NO_SSLv3
+ sslcontext.check_hostname = False
+ sslcontext.verify_mode = ssl.CERT_NONE
+ sslcontext.options |= ssl.OP_NO_COMPRESSION
+ sslcontext.set_default_verify_paths()
+ return sslcontext
+
+ async def _get_ssl_context(self, req: ClientRequest) -> Optional[SSLContext]:
  """Logic to get the correct SSL context
 
  0. if req.ssl is false, return None
@@ -1005,25 +1008,46 @@ def _get_ssl_context(self, req: ClientRequest) -> Optional[SSLContext]:
  3. if verify_ssl is False in req, generate a SSL context that
  won't verify
  """
- if req.is_ssl():
- if ssl is None: # pragma: no cover
- raise RuntimeError("SSL is not supported.")
- sslcontext = req.ssl
- if isinstance(sslcontext, ssl.SSLContext):
- return sslcontext
- if sslcontext is not True:
- # not verified or fingerprinted
- return self._make_ssl_context(False)
- sslcontext = self._ssl
- if isinstance(sslcontext, ssl.SSLContext):
- return sslcontext
- if sslcontext is not True:
- # not verified or fingerprinted
- return self._make_ssl_context(False)
- return self._make_ssl_context(True)
- else:
+ if not req.is_ssl():
  return None
 
+ if ssl is None: # pragma: no cover
+ raise RuntimeError("SSL is not supported.")
+ sslcontext = req.ssl
+ if isinstance(sslcontext, ssl.SSLContext):
+ return sslcontext
+ if sslcontext is not True:
+ # not verified or fingerprinted
+ return await self._make_or_get_ssl_context(False)
+ sslcontext = self._ssl
+ if isinstance(sslcontext, ssl.SSLContext):
+ return sslcontext
+ if sslcontext is not True:
+ # not verified or fingerprinted
+ return await self._make_or_get_ssl_context(False)
+ return await self._make_or_get_ssl_context(True)
+
+ async def _make_or_get_ssl_context(self, verified: bool) -> SSLContext:
+ """Create or get cached SSL context."""
+ try:
+ return await self._made_ssl_context[verified]
+ except KeyError:
+ loop = self._loop
+ future = loop.create_future()
+ self._made_ssl_context[verified] = future
+ try:
+ result = await loop.run_in_executor(
+ None, self._make_ssl_context, verified
+ )
+ # BaseException is used since we might get CancelledError
+ except BaseException as ex:
+ del self._made_ssl_context[verified]
+ set_exception(future, ex)
+ raise
+ else:
+ set_result(future, result)
+ return result
+
  def _get_fingerprint(self, req: ClientRequest) -> Optional["Fingerprint"]:
  ret = req.ssl
  if isinstance(ret, Fingerprint):
@@ -1180,7 +1204,7 @@ async def _start_tls_connection(
  # `req.is_ssl()` evaluates to `False` which is never gonna happen
  # in this code path. Of course, it's rather fragile
  # maintainability-wise but this is to be solved separately.
- sslcontext = cast(ssl.SSLContext, self._get_ssl_context(req))
+ sslcontext = cast(ssl.SSLContext, await self._get_ssl_context(req))
 
  try:
  async with ceil_timeout(
@@ -1258,7 +1282,7 @@ async def _create_direct_connection(
  *,
  client_error: Type[Exception] = ClientConnectorError,
  ) -> Tuple[asyncio.Transport, ResponseHandler]:
- sslcontext = self._get_ssl_context(req)
+ sslcontext = await self._get_ssl_context(req)
  fingerprint = self._get_fingerprint(req)
 
  host = req.url.raw_host

tests/test_connector.py

@@ -1540,23 +1540,23 @@ async def test_tcp_connector_clear_dns_cache_bad_args(loop) -> None:
  conn.clear_dns_cache("localhost")
 
 
-async def test_dont_recreate_ssl_context(loop) -> None:
- conn = aiohttp.TCPConnector(loop=loop)
- ctx = conn._make_ssl_context(True)
- assert ctx is conn._make_ssl_context(True)
+async def test_dont_recreate_ssl_context() -> None:
+ conn = aiohttp.TCPConnector()
+ ctx = await conn._make_or_get_ssl_context(True)
+ assert ctx is await conn._make_or_get_ssl_context(True)
 
 
-async def test_dont_recreate_ssl_context2(loop) -> None:
- conn = aiohttp.TCPConnector(loop=loop)
- ctx = conn._make_ssl_context(False)
- assert ctx is conn._make_ssl_context(False)
+async def test_dont_recreate_ssl_context2() -> None:
+ conn = aiohttp.TCPConnector()
+ ctx = await conn._make_or_get_ssl_context(False)
+ assert ctx is await conn._make_or_get_ssl_context(False)
 
 
-async def test___get_ssl_context1(loop) -> None:
- conn = aiohttp.TCPConnector(loop=loop)
+async def test___get_ssl_context1() -> None:
+ conn = aiohttp.TCPConnector()
  req = mock.Mock()
  req.is_ssl.return_value = False
- assert conn._get_ssl_context(req) is None
+ assert await conn._get_ssl_context(req) is None
 
 
 async def test___get_ssl_context2(loop) -> None:
@@ -1565,7 +1565,7 @@ async def test___get_ssl_context2(loop) -> None:
  req = mock.Mock()
  req.is_ssl.return_value = True
  req.ssl = ctx
- assert conn._get_ssl_context(req) is ctx
+ assert await conn._get_ssl_context(req) is ctx
 
 
 async def test___get_ssl_context3(loop) -> None:
@@ -1574,7 +1574,7 @@ async def test___get_ssl_context3(loop) -> None:
  req = mock.Mock()
  req.is_ssl.return_value = True
  req.ssl = True
- assert conn._get_ssl_context(req) is ctx
+ assert await conn._get_ssl_context(req) is ctx
 
 
 async def test___get_ssl_context4(loop) -> None:
@@ -1583,7 +1583,9 @@ async def test___get_ssl_context4(loop) -> None:
  req = mock.Mock()
  req.is_ssl.return_value = True
  req.ssl = False
- assert conn._get_ssl_context(req) is conn._make_ssl_context(False)
+ assert await conn._get_ssl_context(req) is await conn._make_or_get_ssl_context(
+ False
+ )
 
 
 async def test___get_ssl_context5(loop) -> None:
@@ -1592,15 +1594,55 @@ async def test___get_ssl_context5(loop) -> None:
  req = mock.Mock()
  req.is_ssl.return_value = True
  req.ssl = aiohttp.Fingerprint(hashlib.sha256(b"1").digest())
- assert conn._get_ssl_context(req) is conn._make_ssl_context(False)
+ assert await conn._get_ssl_context(req) is await conn._make_or_get_ssl_context(
+ False
+ )
 
 
-async def test___get_ssl_context6(loop) -> None:
- conn = aiohttp.TCPConnector(loop=loop)
+async def test___get_ssl_context6() -> None:
+ conn = aiohttp.TCPConnector()
+ req = mock.Mock()
+ req.is_ssl.return_value = True
+ req.ssl = True
+ assert await conn._get_ssl_context(req) is await conn._make_or_get_ssl_context(True)
+
+
+async def test_ssl_context_once() -> None:
+ """Test the ssl context is created only once and shared between connectors."""
+ conn1 = aiohttp.TCPConnector()
+ conn2 = aiohttp.TCPConnector()
+ conn3 = aiohttp.TCPConnector()
+
  req = mock.Mock()
  req.is_ssl.return_value = True
  req.ssl = True
- assert conn._get_ssl_context(req) is conn._make_ssl_context(True)
+ assert await conn1._get_ssl_context(req) is await conn1._make_or_get_ssl_context(
+ True
+ )
+ assert await conn2._get_ssl_context(req) is await conn1._make_or_get_ssl_context(
+ True
+ )
+ assert await conn3._get_ssl_context(req) is await conn1._make_or_get_ssl_context(
+ True
+ )
+ assert conn1._made_ssl_context is conn2._made_ssl_context is conn3._made_ssl_context
+ assert True in conn1._made_ssl_context
+
+
+@pytest.mark.parametrize("exception", [OSError, ssl.SSLError, asyncio.CancelledError])
+async def test_ssl_context_creation_raises(exception: BaseException) -> None:
+ """Test that we try again if SSLContext creation fails the first time."""
+ conn = aiohttp.TCPConnector()
+ conn._made_ssl_context.clear()
+
+ with mock.patch.object(
+ conn, "_make_ssl_context", side_effect=exception
+ ), pytest.raises( # type: ignore[call-overload]
+ exception
+ ):
+ await conn._make_or_get_ssl_context(True)
+
+ assert isinstance(await conn._make_or_get_ssl_context(True), ssl.SSLContext)
 
 
 async def test_close_twice(loop) -> None:

tests/test_proxy.py

@@ -817,7 +817,7 @@ async def make_conn():
  self.loop.start_tls.assert_called_with(
  mock.ANY,
  mock.ANY,
- connector._make_ssl_context(True),
+ self.loop.run_until_complete(connector._make_or_get_ssl_context(True)),
  server_hostname="www.python.org",
  ssl_handshake_timeout=mock.ANY,
  )