Repository: aio-libs/aiohttp
Pull request: Fix SSLContext creation in the TCPConnector with multiple loops
Creation of the SSLContexts did not account for multiple event loops in different threads.

Creation is now done at import time to ensure it does not block the event loop.

fixes #9020


Problem statement:
Incorrect re-use future in _make_or_get_ssl_context
### Describe the bug

I meet following error:
```
RuntimeError: Task <Task pending name='Task-2' coro=<async_connect() running at <ipython-input-1-ba65b5596d34>:18>> got Future <Future pending> attached to a different loop
```
After going through the code, it seems the bug is caused by `aiohttp/connector.py::_make_or_get_ssl_context`. There is a global cache _made_ssl_context for asyncio.Future[SSLContext]. However, the cached future is bound to the first loop. When a new call with different loop to _make_or_get_ssl_context, the cached future is not expected because it has the old loop.

The bug exists since 3.10

### To Reproduce

Please see the description.

### Expected behavior

The calls to `client_session.ws_connect(url)` by multiple threads should not throw exception

### Logs/tracebacks

```python-traceback
RuntimeError: Task <Task pending name='Task-2' coro=<async_connect() running at <ipython-input-1-ba65b5596d34>:18>> got Future <Future pending> attached to a different loop
```


### Python Version

```console
$ python --version
3.8
```


### aiohttp Version

```console
$ python -m pip show aiohttp
3.10.5
```


### multidict Version

```console
$ python -m pip show multidict
n/a
```


### yarl Version

```console
$ python -m pip show yarl
n/a
```


### OS

Linux

### Related component

Client

### Additional context

N/A

### Code of Conduct

- [X] I agree to follow the aio-libs Code of Conduct

Hints from issue discussion:
Thats definitely going to be a problem.

This one might be a bit tricky to fix.
It would be very helpful to get to a solution quicker if you could provide an xfail test for this case.
Generally, you shouldn't have multiple loops, which is probably why nobody else has noticed the issue so far.

But, I agree this looks like it should be isolated. @bdraco Shouldn't it just be a cache on an instance, rather than a class variable? Do we need to cache between connectors? (most users only use a single connector for an entire application anyway)
Creating an ssl context is relatively expensive.  I think it’s more common to have a lot of connectors. At least on my Home Assistant installs I see some with up to 40 as each library is making their own.
The simplest and most straightforward fix would be to create the verify and no verify context at import time. The downside is that we end up doing work that may never get used if they don’t make any ssl connections 
OK, that'd be difficult to deal with then. If we want to handle this case, then we'd need to make that cache loop-local somehow. Maybe a simple solution would be to do a nested dict (`{loop_id: {...}`). Though I'm not clear how much benefit there is in supporting multiple loops.
> The simplest and most straightforward fix would be to create the verify and no verify context at import time. The downside is that we end up doing work that may never get used if they don’t make any ssl connections

Oh, I forgot it was only the 2 contexts. That would also work, and avoid recomputing per loop.

Commit message: make sure test passes with http and not https

Patch to review:
diff --git a/tests/test_connector.py b/tests/test_connector.py
index d66bc214ed2..2873645be6b 100644
--- a/tests/test_connector.py
+++ b/tests/test_connector.py
@@ -9,6 +9,7 @@
 import sys
 import uuid
 from collections import deque
+from concurrent.futures import ThreadPoolExecutor, as_completed
 from contextlib import closing, suppress
 from typing import (
     Awaitable,
@@ -16,6 +17,7 @@
     Dict,
     Iterator,
     List,
+    Literal,
     NoReturn,
     Optional,
     Sequence,
@@ -2977,3 +2979,36 @@ async def allow_connection_and_add_dummy_waiter() -> None:
             )
 
             await connector.close()
+
+
+@pytest.mark.xfail
+def test_connector_multiple_event_loop() -> None:
+    """Test the connector with multiple event loops."""
+
+    async def async_connect() -> Literal[True]:
+        conn = aiohttp.TCPConnector()
+        loop = asyncio.get_running_loop()
+        req = ClientRequest("GET", URL("https://127.0.0.1"), loop=loop)
+        with suppress(aiohttp.ClientConnectorError):
+            with mock.patch.object(
+                conn._loop,
+                "create_connection",
+                autospec=True,
+                spec_set=True,
+                side_effect=ssl.CertificateError,
+            ):
+                await conn.connect(req, [], ClientTimeout())
+        return True
+
+    def test_connect() -> Literal[True]:
+        loop = asyncio.new_event_loop()
+        try:
+            return loop.run_until_complete(async_connect())
+        finally:
+            loop.close()
+
+    with ThreadPoolExecutor() as executor:
+        res_list = [executor.submit(test_connect) for _ in range(2)]
+        raw_response_list = [res.result() for res in as_completed(res_list)]
+
+    assert raw_response_list == [True, True]


Code review comments:
diff_hunk: @@ -9,13 +9,15 @@
 import sys
 import uuid
 from collections import deque
+from concurrent.futures import ThreadPoolExecutor, as_completed
path: tests/test_connector.py
text: @user1:
I'd suggest import `futures` given that as_completed() also exists in asyncio and could cause confusion.