From fbc386485bc841d0f73ff7e38d942845dc31a0c0 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:56:46 +0300 Subject: [PATCH 1/8] Fix AttributeError in HTTPServer._serve_unservicable() error handler Fixes #797. The except-block that's meant to log an unexpected failure while sending a 503 response called self.server.error_log(...), but self is the HTTPServer instance itself here, and HTTPServer has no server attribute -- that belongs to a different class, HTTPConnection (whose self.server does point back to its owning HTTPServer). So instead of logging the original exception and keeping the background thread alive (which is exactly what the comment right above says this code is for), it raised a fresh AttributeError from inside the except-block. Call self.error_log(...) directly, matching HTTPServer's own method. --- cheroot/server.py | 2 +- cheroot/test/test_server.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/cheroot/server.py b/cheroot/server.py index 284cf17c72..1a0970076d 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1904,7 +1904,7 @@ def _serve_unservicable(self): # We can't just raise an exception because that will kill this # thread, and prevent 503 errors from being sent to future # connections. - self.server.error_log( + self.error_log( repr(ex), level=logging.ERROR, traceback=True, diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index ae6e390a44..53f1688836 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -616,6 +616,35 @@ def test_overload_results_in_suitable_http_error(request): assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE +def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys): + """A failure while sending a 503 must be logged, not raise AttributeError. + + Regression test: ``HTTPServer._serve_unservicable()`` called + ``self.server.error_log(...)``, but ``HTTPServer`` has no ``server`` + attribute -- that belongs to ``HTTPConnection``, which is a different + class. So instead of logging the original failure, it raised a fresh + ``AttributeError`` from within the except-block meant to keep this + background thread alive. See issue #797. + """ + from .. import server as server_module + + httpserver = HTTPServer.__new__(HTTPServer) + httpserver.ready = True + httpserver._unservicable_conns = queue.Queue() + fake_conn = mocker.Mock() + httpserver._unservicable_conns.put(fake_conn) + httpserver._unservicable_conns.put(server_module._STOPPING_FOR_INTERRUPT) + + fake_request = mocker.Mock() + fake_request.simple_response.side_effect = ValueError('boom') + mocker.patch.object(server_module, 'HTTPRequest', return_value=fake_request) + + httpserver._serve_unservicable() # must not raise + + assert 'boom' in capsys.readouterr().err + fake_conn.close.assert_called_once() + + def test_overload_thread_does_not_leak(): """On shutdown the overload thread exits. From d73e5a1c4d21722a30f9b474605e56a7381ac6a6 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Thu, 17 Sep 2026 15:57:17 +0300 Subject: [PATCH 2/8] Add changelog fragment for #797 Cheroot requires a Towncrier news fragment for every user-visible change. This adds the bugfix entry for the AttributeError that HTTPServer._serve_unservicable() raised from its own error handler, so the fix shows up in the release notes. --- docs/changelog-fragments.d/797.bugfix.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog-fragments.d/797.bugfix.rst diff --git a/docs/changelog-fragments.d/797.bugfix.rst b/docs/changelog-fragments.d/797.bugfix.rst new file mode 100644 index 0000000000..58846fd3d8 --- /dev/null +++ b/docs/changelog-fragments.d/797.bugfix.rst @@ -0,0 +1,5 @@ +Fixed :py:meth:`HTTPServer._serve_unservicable() ` +raising an unrelated :py:exc:`AttributeError` instead of logging the original failure +when sending a ``503 Service Unavailable`` response itself raised an unexpected exception. + +-- by :user:`Tatamis` From 878e3aa7d92f768d6809b43f716c324b30954332 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:58:48 +0000 Subject: [PATCH 3/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- cheroot/test/test_server.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index 53f1688836..cdba6fea2f 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -637,7 +637,11 @@ def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys): fake_request = mocker.Mock() fake_request.simple_response.side_effect = ValueError('boom') - mocker.patch.object(server_module, 'HTTPRequest', return_value=fake_request) + mocker.patch.object( + server_module, + 'HTTPRequest', + return_value=fake_request, + ) httpserver._serve_unservicable() # must not raise From 9e7e3c06d0cd0328eda334af9f435974867216a4 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:41:01 +0300 Subject: [PATCH 4/8] Address review feedback on the unservicable-connection test julianz- pointed out the regression test imported the whole `server` module locally just to reach two names. _STOPPING_FOR_INTERRUPT moves into the top-level `from ..server import (...)` block, and the HTTPRequest patch now goes through mocker.patch('cheroot.server.HTTPRequest', ...) directly instead of patching an attribute on an imported module object. Also picked up the suggested comma in the changelog fragment. --- cheroot/test/test_server.py | 17 ++++++++--------- docs/changelog-fragments.d/797.bugfix.rst | 2 +- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index cdba6fea2f..fa275a3927 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -20,7 +20,12 @@ from pypytools.gc.custom import DefaultGc from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM, bton, ntob -from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPServer +from ..server import ( + _STOPPING_FOR_INTERRUPT, + IS_UID_GID_RESOLVABLE, + Gateway, + HTTPServer, +) from ..testing import ( ANY_INTERFACE_IPV4, ANY_INTERFACE_IPV6, @@ -626,22 +631,16 @@ def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys): ``AttributeError`` from within the except-block meant to keep this background thread alive. See issue #797. """ - from .. import server as server_module - httpserver = HTTPServer.__new__(HTTPServer) httpserver.ready = True httpserver._unservicable_conns = queue.Queue() fake_conn = mocker.Mock() httpserver._unservicable_conns.put(fake_conn) - httpserver._unservicable_conns.put(server_module._STOPPING_FOR_INTERRUPT) + httpserver._unservicable_conns.put(_STOPPING_FOR_INTERRUPT) fake_request = mocker.Mock() fake_request.simple_response.side_effect = ValueError('boom') - mocker.patch.object( - server_module, - 'HTTPRequest', - return_value=fake_request, - ) + mocker.patch('cheroot.server.HTTPRequest', return_value=fake_request) httpserver._serve_unservicable() # must not raise diff --git a/docs/changelog-fragments.d/797.bugfix.rst b/docs/changelog-fragments.d/797.bugfix.rst index 58846fd3d8..d3d4601c3c 100644 --- a/docs/changelog-fragments.d/797.bugfix.rst +++ b/docs/changelog-fragments.d/797.bugfix.rst @@ -1,5 +1,5 @@ Fixed :py:meth:`HTTPServer._serve_unservicable() ` -raising an unrelated :py:exc:`AttributeError` instead of logging the original failure +raising an unrelated :py:exc:`AttributeError` instead of logging the original failure, when sending a ``503 Service Unavailable`` response itself raised an unexpected exception. -- by :user:`Tatamis` From c80b3cc6b4ce16ad382276c27ef6d74d741fdb53 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Sat, 19 Sep 2026 09:41:25 +0300 Subject: [PATCH 5/8] Rewrite the 503 failure test as an end-to-end one Run the test against a real overloaded server instead of mocking the internals of `HTTPServer._serve_unservicable()`. Only the failure to send the 503 response is simulated. The test checks that the original error is logged and that later connections still get their 503, which is the behavior that broke with the `AttributeError`. Move the overload server setup into a fixture shared with `test_overload_results_in_suitable_http_error`, and add type annotations to the new and touched test functions. The approach draws on the tests from #825 and #830. Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com> --- cheroot/test/test_server.py | 102 ++++++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 38 deletions(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index fa275a3927..23a044bf6c 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -9,6 +9,7 @@ import tempfile import threading import types +import typing as _t import urllib.parse # noqa: WPS301 import uuid from http import HTTPStatus @@ -20,12 +21,7 @@ from pypytools.gc.custom import DefaultGc from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM, bton, ntob -from ..server import ( - _STOPPING_FOR_INTERRUPT, - IS_UID_GID_RESOLVABLE, - Gateway, - HTTPServer, -) +from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPRequest, HTTPServer from ..testing import ( ANY_INTERFACE_IPV4, ANY_INTERFACE_IPV6, @@ -588,11 +584,11 @@ def test_threadpool_multistart_validation(monkeypatch): tp.start() -def test_overload_results_in_suitable_http_error(request): - """A server that can't keep up with requests returns a 503 HTTP error.""" - localhost = '127.0.0.1' +@pytest.fixture +def overloaded_http_server() -> _t.Iterator[HTTPServer]: + """Return a running server that answers every request with a 503.""" httpserver = HTTPServer( - bind_addr=(localhost, EPHEMERAL_PORT), + bind_addr=('127.0.0.1', EPHEMERAL_PORT), gateway=Gateway, ) # Can only handle on request in parallel: @@ -607,45 +603,75 @@ def test_overload_results_in_suitable_http_error(request): httpserver.prepare() serve_thread = threading.Thread(target=httpserver.serve) serve_thread.start() - request.addfinalizer(httpserver.stop) - # Stop the thread pool to ensure the queue fills up: - httpserver.requests.stop() + try: + # Stop the thread pool to ensure the queue fills up: + httpserver.requests.stop() + + # Use up the very limited thread pool queue we've set up, so future + # requests fail: + httpserver.requests._queue.put(None) + + yield httpserver + finally: + httpserver.stop() - _host, port = httpserver.bind_addr - # Use up the very limited thread pool queue we've set up, so future - # requests fail: - httpserver.requests._queue.put(None) +def test_overload_results_in_suitable_http_error( + overloaded_http_server: HTTPServer, +) -> None: + """A server that can't keep up with requests returns a 503 HTTP error.""" + host, port = overloaded_http_server.bind_addr - response = requests.get(f'http://{localhost}:{port}', timeout=20) + response = requests.get(f'http://{host}:{port}', timeout=20) assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE -def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys): - """A failure while sending a 503 must be logged, not raise AttributeError. +def test_overload_survives_failure_to_send_http_error( + overloaded_http_server: HTTPServer, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """An unexpected error while sending a 503 is logged, not fatal. + + The overload handling must keep on answering later connections. - Regression test: ``HTTPServer._serve_unservicable()`` called - ``self.server.error_log(...)``, but ``HTTPServer`` has no ``server`` - attribute -- that belongs to ``HTTPConnection``, which is a different - class. So instead of logging the original failure, it raised a fresh - ``AttributeError`` from within the except-block meant to keep this - background thread alive. See issue #797. + This is a test for issue #797. """ - httpserver = HTTPServer.__new__(HTTPServer) - httpserver.ready = True - httpserver._unservicable_conns = queue.Queue() - fake_conn = mocker.Mock() - httpserver._unservicable_conns.put(fake_conn) - httpserver._unservicable_conns.put(_STOPPING_FOR_INTERRUPT) + host, port = overloaded_http_server.bind_addr + real_simple_response = HTTPRequest.simple_response + attempts = [] + + def failing_once_simple_response( + request: HTTPRequest, + status: str, + msg: str = '', + ) -> None: + attempts.append(status) + if len(attempts) == 1: + raise RuntimeError('unexpected failure sending the 503') + real_simple_response(request, status, msg) - fake_request = mocker.Mock() - fake_request.simple_response.side_effect = ValueError('boom') - mocker.patch('cheroot.server.HTTPRequest', return_value=fake_request) + monkeypatch.setattr( + HTTPRequest, + 'simple_response', + failing_once_simple_response, + ) - httpserver._serve_unservicable() # must not raise + # The server fails to send the 503 to this connection and leaves it + # lingering, so the client won't ever get a response over it: + with socket.create_connection((host, port), timeout=20): + # The overload thread handles connections one by one, so once this + # one is answered, the failed attempt above has been dealt with: + response = requests.get(f'http://{host}:{port}', timeout=20) - assert 'boom' in capsys.readouterr().err - fake_conn.close.assert_called_once() + assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE + + # The original failure is reported instead of a bogus `AttributeError`: + captured_stderr = capsys.readouterr().err + assert ( + 'RuntimeError: unexpected failure sending the 503' in captured_stderr + ) + assert 'AttributeError' not in captured_stderr def test_overload_thread_does_not_leak(): From 2753fd9c7e76ae33db092da3d6f6924575da0752 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Sat, 19 Sep 2026 09:41:25 +0300 Subject: [PATCH 6/8] Reword the #797 changelog fragment for end users Describe the visible effect of the bug rather than the internals, and credit the authors of the earlier fixes from #825 and #830. Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com> --- docs/changelog-fragments.d/797.bugfix.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/changelog-fragments.d/797.bugfix.rst b/docs/changelog-fragments.d/797.bugfix.rst index d3d4601c3c..1f115ba775 100644 --- a/docs/changelog-fragments.d/797.bugfix.rst +++ b/docs/changelog-fragments.d/797.bugfix.rst @@ -1,5 +1,5 @@ -Fixed :py:meth:`HTTPServer._serve_unservicable() ` -raising an unrelated :py:exc:`AttributeError` instead of logging the original failure, -when sending a ``503 Service Unavailable`` response itself raised an unexpected exception. +Fixed the server stopping to answer overloaded connections with +``503 Service Unavailable`` after a failure to send one of these responses. +The failure is now logged and the following connections are still answered. --- by :user:`Tatamis` +-- by :user:`Tatamis`, :user:`avinashkamat48` and :user:`andrewkernel` From 8c6e9abae80b2ae1a4e57bcaa6534c60c8b66bb1 Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Sun, 20 Sep 2026 05:57:48 +0300 Subject: [PATCH 7/8] Ignore unresolvable pytest types in the docs build The test modules are documented with `autodoc`, and the new test annotations refer to `pytest.MonkeyPatch` and `pytest.CaptureFixture`. Sphinx resolves these to the private `_pytest` package, which has no entries in any inventory, so the nitpicky docs build fails. Ignore them like the other unresolvable references. --- docs/conf.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/conf.py b/docs/conf.py index 1d526cae44..e97a905e73 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -150,6 +150,10 @@ ('py:class', '_pyio.BufferedWriter'), ('py:class', '_pyio.BufferedReader'), ('py:class', 'unittest.case.TestCase'), + # Pytest exposes these under the public `pytest.` names but the + # documented objects live in its private `_pytest` package: + ('py:class', '_pytest.capture.CaptureFixture'), + ('py:class', '_pytest.monkeypatch.MonkeyPatch'), ('py:meth', 'cheroot.connections.ConnectionManager.get_conn'), # Ref: https://github.com/pyca/pyopenssl/issues/1012 ('py:class', 'pyopenssl:OpenSSL.SSL.Context'), From b9967da7b14e447f513fd88546fc4f4d9685c39f Mon Sep 17 00:00:00 2001 From: Tatamis <80774326+Tatamis@users.noreply.github.com> Date: Sun, 20 Sep 2026 05:57:48 +0300 Subject: [PATCH 8/8] Use the suggested wording in the #797 changelog fragment Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com> --- docs/changelog-fragments.d/797.bugfix.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/changelog-fragments.d/797.bugfix.rst b/docs/changelog-fragments.d/797.bugfix.rst index 1f115ba775..4ff21f45e0 100644 --- a/docs/changelog-fragments.d/797.bugfix.rst +++ b/docs/changelog-fragments.d/797.bugfix.rst @@ -1,5 +1,7 @@ -Fixed the server stopping to answer overloaded connections with -``503 Service Unavailable`` after a failure to send one of these responses. -The failure is now logged and the following connections are still answered. +Fixed the server's ability to send ``503 Service Unavailable`` whenever the +server becomes too busy. Failures to send caused the handler for this message +to crash, leaving new connections during busy times unanswered. Any failure in +sending this message is now logged, and subsequent connections continue to be +answered. -- by :user:`Tatamis`, :user:`avinashkamat48` and :user:`andrewkernel`