Skip to content
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

test_ssl: ConnectionHandler shuts down ThreadedEchoServer #126500

Closed
encukou opened this issue Nov 6, 2024 · 1 comment
Closed

test_ssl: ConnectionHandler shuts down ThreadedEchoServer #126500

encukou opened this issue Nov 6, 2024 · 1 comment
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
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
2 participants
@encukou @ZeroIntensity