Skip to content

test_ssl: ConnectionHandler shuts down ThreadedEchoServer #126500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
encukou opened this issue Nov 6, 2024 · 3 comments
Closed

test_ssl: ConnectionHandler shuts down ThreadedEchoServer #126500

encukou opened this issue Nov 6, 2024 · 3 comments
Assignees
@encukou
Labels
tests Tests in the Lib/test dir topic-SSL

Comments

@encukou
Copy link
Member

encukou commented Nov 6, 2024 •
edited by bedevere-app bot
Loading

Recently, test_ssl has been failing intermittently but frequently on macOS buildbots, see for example here (but look at the first failure; the re-run is #126499).

If read() in a ThreadedEchoServer's ConnectionHandler thread raises OSError (except ConnectionError), the ConnectionHandler shuts down the entire server, preventing further connections.

As far as I can see, this is done to avoid the server thread getting stuck, forgotten, in its accept loop. However, since 2011 (5b95eb9) the server is used as a context manager, and its __exit__ does stop() and join().
(I'm not sure if we always used with since that commit, but currently we do.)

The most common failure I saw is the following. Note that it's the third connect call in this test: presumably, if the server thread started shutting down after the first connect it had enough time by now. But sometimes the same happens with the second connect, in this test or another one.

Traceback (most recent call last):
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/test/test_ssl.py", line 1897, in test_connect_with_context
    s.connect(self.server_addr)
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/ssl.py", line 1405, in connect
    self._real_connect(addr, False)
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
  File "/Users/buildbot/buildarea/3.x.pablogsal-macos-m1.macos-with-brew/build/Lib/ssl.py", line 1392, in _real_connect
    super().connect(addr)
    ~~~~~~~~~~~~~~~^^^^^^
ConnectionRefusedError: [Errno 61] Connection refused

I'll send a PR soon.

Linked PRs
@encukou encukou self-assigned this Nov 6, 2024
@ZeroIntensity ZeroIntensity added tests Tests in the Lib/test dir topic-SSL labels Nov 6, 2024
encukou added a commit to encukou/cpython that referenced this issue Nov 6, 2024
@encukou
pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError i…
aa65fb4 
…n ConnectionHandler; rely on __exit__

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
@bedevere-app bedevere-app bot mentioned this issue Nov 6, 2024
Merged
encukou added a commit that referenced this issue Nov 7, 2024
@encukou
gh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError in Conn…
c9cda16 
…ectionHandler; rely on __exit__ (GH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
@encukou
Copy link
Member Author

encukou commented Nov 7, 2024

I'll watch the buildbot for a day or two before backporting the fix.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 8, 2024
@encukou @miss-islington
pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError i…
dd288da 
…n ConnectionHandler; rely on __exit__ (pythonGH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
(cherry picked from commit c9cda16)

Co-authored-by: Petr Viktorin <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 8, 2024
@encukou @miss-islington
pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError i…
3d39014 
…n ConnectionHandler; rely on __exit__ (pythonGH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
(cherry picked from commit c9cda16)

Co-authored-by: Petr Viktorin <[email protected]>
This was referenced Nov 8, 2024
Merged
Merged
encukou added a commit that referenced this issue Nov 11, 2024
@miss-islington @encukou
[3.13] gh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError …
fc10908 
…in ConnectionHandler; rely on __exit__ (GH-126503) (GH-126571)

gh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError in ConnectionHandler; rely on __exit__ (GH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
(cherry picked from commit c9cda16)

Co-authored-by: Petr Viktorin <[email protected]>
encukou added a commit that referenced this issue Nov 11, 2024
@miss-islington @encukou
[3.12] gh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError …
aee80cd 
…in ConnectionHandler; rely on __exit__ (GH-126503) (GH-126572)

gh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError in ConnectionHandler; rely on __exit__ (GH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
(cherry picked from commit c9cda16)

Co-authored-by: Petr Viktorin <[email protected]>
@encukou encukou closed this as completed Nov 11, 2024
@github-actions github-actions bot mentioned this issue Dec 1, 2024
Closed
picnixz pushed a commit to picnixz/cpython that referenced this issue Dec 8, 2024
@encukou @picnixz
pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError i…
840b2a8 
…n ConnectionHandler; rely on __exit__ (pythonGH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
ebonnal pushed a commit to ebonnal/cpython that referenced this issue Jan 12, 2025
@encukou @ebonnal
pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError i…
593dd72 
…n ConnectionHandler; rely on __exit__ (pythonGH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
@stratakis
Copy link
Contributor

stratakis commented Apr 14, 2025 •
edited
Loading

We started noticing these failures consistently on Python 3.11 and 3.10 when Fedora Rawhide updated to OpenSSL 3.5. It's fairly easy to reproduce, just compile python3.11 or 3.10 on a rawhide machine and run test_ssl.

Pythons <= 3.9 do not seem affected.

You can also see the failures on the rawhide buildbots e.g. here: https://buildbot.python.org/all/#/builders/965/builds/1163

Not exactly certain of the root cause, however the relevant PR resolves the issue.

Given that 3.10 and 3.11 are on a security fix mode only, I think this would be a good candidate to backport as it resolves the issue with the latest OpenSSL. Similarly a different issue with OpenSSL was backported as well to these releases:
#127361

@encukou what do you think?

@jamadden jamadden mentioned this issue Apr 24, 2025
Open
miss-islington added a commit to miss-islington/cpython that referenced this issue Apr 29, 2025
@miss-islington @encukou
[3.12] pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OS…
683380e 
…Error in ConnectionHandler; rely on __exit__ (pythonGH-126503) (pythonGH-126572)

pythongh-126500: test_ssl: Don't stop ThreadedEchoServer on OSError in ConnectionHandler; rely on __exit__ (pythonGH-126503)

If `read()` in the ConnectionHandler thread raises `OSError` (except `ConnectionError`),
the ConnectionHandler shuts down the entire ThreadedEchoServer,
preventing further connections.
It also does that for `EPROTOTYPE` in `wrap_conn`.

As far as I can see, this is done to avoid the server thread getting stuck,
forgotten, in its accept loop. However, since 2011 (5b95eb9)
the server is used as a context manager, and its `__exit__` does `stop()` and `join()`.
(I'm not sure if we *always* used `with` since that commit, but currently we do.)

Make sure that the context manager *is* used, and remove the `server.stop()`
calls from ConnectionHandler.
(cherry picked from commit c9cda16)

(cherry picked from commit aee80cd)

Co-authored-by: Miss Islington (bot) <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>
@miss-islington miss-islington mentioned this issue Apr 29, 2025
Open
@encukou
Copy link
Member Author

encukou commented Apr 29, 2025

Not my call; let me prepare the backport and ask the release managers.
Thanks for the ping; this slipped from my radar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@encukou encukou

Labels
tests Tests in the Lib/test dir topic-SSL
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@encukou @stratakis @ZeroIntensity