Fix AttributeError in the unserviceable-connection error handler by Tatamis · Pull Request #840 · cherrypy/cheroot

Tatamis · 2026-09-17T12:57:35Z

What kind of change does this PR introduce?

What do these changes do?

HTTPServer._serve_unservicable() catches unexpected failures while sending a
503 Service Unavailable response and logs them, deliberately, so that the
background thread stays alive and later connections still get their 503:

except Exception as ex:
    # 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(
        repr(ex),
        level=logging.ERROR,
        traceback=True,
    )

The problem is self.server. Here self is the HTTPServer itself, and
HTTPServer has no server attribute — that belongs to HTTPConnection,
whose self.server does point back at its owning HTTPServer. The two other
self.server.error_log(...) calls in this file live in
HTTPConnection.communicate(), where they are correct.

So the handler raises a fresh AttributeError from inside the except-block,
the original error is never logged, and the thread that is supposed to keep
answering overloaded connections dies — the exact opposite of what the comment
above it promises.

The fix is one line: call self.error_log(...), which HTTPServer defines a
few dozen lines further down in the same class.

I also added test_overload_survives_failure_to_send_http_error, an
end-to-end test that runs against a real overloaded server. Only the failure
to send the 503 is simulated, by making HTTPRequest.simple_response() raise
once. It checks that the original error is logged rather than an
AttributeError, and that the next client still receives its 503. The
overload server setup now lives in a fixture that
test_overload_results_in_suitable_http_error shares. The test design draws
on #825 and #830, whose authors are credited in the commits and in the change
log entry.

I should mention: you noted this is the third PR for this bug, after #825 and
#830, and I only saw those after your comment. The one-line fix is necessarily
the same in all three. If you would rather land one of the earlier PRs, please
do — I am happy for this one to be closed, and can port the test over there
instead if that helps.

Are there changes in behavior for the user?

Nothing changes on the happy path. When sending a 503 fails, the original
exception is now written to the error log with its traceback, and the
unserviceable-connection thread keeps running instead of dying with an
unrelated AttributeError. Anyone who wrote code against that AttributeError
(unlikely — it was a crash, not an API) would notice it is gone.

Is it a substantial burden for the maintainers to support this?

No. It swaps one attribute lookup for the method on the same class and adds a
test in the existing style of test_server.py. There is nothing new to keep
working over the next five years, and the test pins the behaviour so the same
mistake cannot come back unnoticed.

Related issue number

Fixes #797. Same underlying bug as #825 and #830.

Checklist

Test run

pytest cheroot/test/test_server.py cheroot/test/test_conn.py cheroot/test/test_core.py
passes on Python 3.8 and 3.13 (95 passed, 11 skipped, 4 xfailed on each).
The new test fails without the fix (the overload thread dies and the
second client never gets its 503) and passes with it.
The repository's pre-commit hooks (flake8 with its plugins, ruff,
mypy, pylint) pass on the changed files.

Fixes cherrypy#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.

read-the-docs-community · 2026-09-17T12:58:41Z

Documentation build overview

📚 cheroot | 🛠️ Build #34655785 | 📁 Comparing b9967da against latest (edef8ff)

🔍 Preview build

12 files changed · ± 12 modified

± Modified

codecov · 2026-09-17T13:02:56Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.36%. Comparing base (edef8ff) to head (b9967da).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
- Coverage   78.37%   78.36%   -0.02%     
==========================================
  Files          41       41              
  Lines        4791     4811      +20     
  Branches      548      549       +1     
==========================================
+ Hits         3755     3770      +15     
- Misses        899      902       +3     
- Partials      137      139       +2

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.

for more information, see https://pre-commit.ci

julianz- · 2026-09-17T21:01:00Z

@Tatamis Thanks for the submission. This is actually the third time a fix for this problem has been submitted: see #825 and #830, both still open. These were incomplete submissions for which I left comments on how to complete. Your submission is also incomplete in that it does not use our template for new PRs - PULL_REQUEST_TEMPLATE.md although you may not have seen this if you used an automated tool for making the submission. Anyway, if you could revise the PR using this template we can take things from there.

julianz- · 2026-09-18T07:32:16Z

+    ``AttributeError`` from within the except-block meant to keep this
+    background thread alive. See issue #797.
+    """
+    from .. import server as server_module


This kind of local, whole-module import shouldn't be needed. _STOPPING_FOR_INTERRUPT can just move into the top-level from ..server import ... block. For HTTPRequest, rather than importing the module just to patch an attribute on it, use mocker.patch('cheroot.server.HTTPRequest', ...) instead — no import needed at all.

julianz- · 2026-09-18T07:59:03Z

@@ -0,0 +1,5 @@
+Fixed :py:meth:`HTTPServer._serve_unservicable() <cheroot.server.HTTPServer._serve_unservicable>`
+raising an unrelated :py:exc:`AttributeError` instead of logging the original failure


Suggested change

raising an unrelated :py:exc:`AttributeError` instead of logging the original failure

raising an unrelated :py:exc:`AttributeError` instead of logging the original failure,

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.

Tatamis · 2026-09-18T11:32:28Z

Pushed 9e7e3c0 addressing this: _STOPPING_FOR_INTERRUPT now comes from the top-level from ..server import (...) block, and the HTTPRequest patch is mocker.patch('cheroot.server.HTTPRequest', ...) instead of patching an attribute on the imported module. Also picked up the comma in the changelog fragment.

julianz- · 2026-09-18T19:27:39Z

Thanks for update. @webknjaz please take a look.

webknjaz · 2026-09-19T00:29:29Z

    assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE


+def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys):


Add type annotations to all newly added test functions.

webknjaz · 2026-09-19T00:30:56Z

+    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 sounds more like a code comment rather than a docsring, which has a different audience and purpose.

webknjaz · 2026-09-19T00:41:05Z

+    ``AttributeError`` from within the except-block meant to keep this
+    background thread alive. See issue #797.
+    """
+    httpserver = HTTPServer.__new__(HTTPServer)


This object creation style doesn't seem to exist anywhere. I'd rather not use it.

webknjaz · 2026-09-19T00:42:22Z

+    httpserver = HTTPServer.__new__(HTTPServer)
+    httpserver.ready = True
+    httpserver._unservicable_conns = queue.Queue()
+    fake_conn = mocker.Mock()


Please don't mock everything. We need to use proper e2e tests as much as possible, only mocking external things/side effects where appropriate.

webknjaz · 2026-09-19T00:44:37Z

Draw inspiration from #825 / #830 to design a better test with stubs, not mocking everything to the point that the mocks are tested more than the overall behavior.

webknjaz · 2026-09-19T00:45:37Z

+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`


We should probably credit the contributors from #825 and #830 in the test rewrite commit and here: https://hynek.me/til/easier-crediting-contributors-github/

webknjaz · 2026-09-19T00:47:22Z

Release notes are generally supposed to be targeting the end-users. This seems to be focusing on the low-level internals. Could you think of making this text more high-level?

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 cherrypy#825 and cherrypy#830. Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com>

Describe the visible effect of the bug rather than the internals, and credit the authors of the earlier fixes from cherrypy#825 and cherrypy#830. Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com>

Tatamis · 2026-09-19T06:50:12Z

@webknjaz thanks for the detailed review. I pushed 2753fd9 (two commits) that addresses all of it:

Mocking / e2e: the test is rewritten as test_overload_survives_failure_to_send_http_error. It runs against a real overloaded server with real sockets. The only thing simulated is the failure to send the 503 (HTTPRequest.simple_response() raises once). It asserts that the original error is logged, with no AttributeError, and that the next client still gets its 503. Design inspired by Fix error logging when sending 503 response fails #825 and Fix unservicable connection error logging #830. It fails without the fix and passes with it.
Object creation style: gone, no hand-built server objects or Mock stand-ins any more. The overload server setup is a fixture now, shared with test_overload_results_in_suitable_http_error.
Type annotations: added to the new fixture and test functions, and to the existing overload test that now uses the fixture.
Docstring vs. comment: the long explanation is gone. The docstring now only says what the test checks, with a reference to Replace self.server.error_log() with self.error_log() in HTTPServer._serve_unservicable() #797.
Credit for Fix error logging when sending 503 response fails #825 / Fix unservicable connection error logging #830: both commits carry Co-authored-by trailers for @avinashkamat48 and @andrewkernel, and the change log fragment credits them too.
Release notes wording: rewritten around what users see (the server stops answering overloaded connections with 503 after a failed send), without the internals.

I ran the changed tests on Python 3.8 and 3.13, and the repo's pre-commit hooks (flake8 with wemake, ruff, mypy, pylint) pass on the changed files. The PR description is updated to match. Please take another look when you have time.

julianz- · 2026-09-19T18:38:44Z

+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`, :user:`avinashkamat48` and :user:`andrewkernel`


Suggested change

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`, :user:`avinashkamat48` and :user:`andrewkernel`

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`

Think this easier to understand as an end user even if it still hints at the internal message handler.

Thanks, applied as suggested in b9967da (rewrapped the lines only). That commit also fixes the docs build: the new test annotations refer to pytest.MonkeyPatch and pytest.CaptureFixture, which Sphinx can't resolve in nitpicky mode, so I added them to nitpick_ignore, like the other unresolvable references.

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.

Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com>

psf-chronographer Bot added the bot:chronographer:provided A mark meaning that a new change log entry is present within the patch. label Sep 17, 2026

webknjaz added the ai-slop LLM-generated interactions with no fact-checking and inauthentic activity label Sep 17, 2026

webknjaz assigned julianz- Sep 17, 2026

webknjaz mentioned this pull request Sep 17, 2026

Replace self.server.error_log() with self.error_log() in HTTPServer._serve_unservicable() #797

Open

Tatamis and others added 2 commits September 17, 2026 18:59

Add changelog fragment for cherrypy#797

d73e5a1

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.

[pre-commit.ci] auto fixes from pre-commit.com hooks

878e3aa

for more information, see https://pre-commit.ci

Tatamis force-pushed the fix/serve-unservicable-error-log branch from 3e98d66 to 878e3aa Compare September 17, 2026 15:59

Tatamis changed the title ~~Fix AttributeError in HTTPServer._serve_unservicable() error handler~~ Fix AttributeError in the unserviceable-connection error handler Sep 17, 2026

julianz- reviewed Sep 18, 2026

View reviewed changes

julianz- assigned webknjaz Sep 18, 2026

webknjaz reviewed Sep 19, 2026

View reviewed changes

Tatamis and others added 2 commits September 19, 2026 09:41

julianz- reviewed Sep 19, 2026

View reviewed changes

Tatamis and others added 2 commits September 20, 2026 05:57

Use the suggested wording in the cherrypy#797 changelog fragment

b9967da

Co-authored-by: era <283892076+avinashkamat48@users.noreply.github.com> Co-authored-by: Andrew Dang <112019350+andrewkernel@users.noreply.github.com>

		@@ -0,0 +1,5 @@
		Fixed :py:meth:`HTTPServer._serve_unservicable() <cheroot.server.HTTPServer._serve_unservicable>`
		raising an unrelated :py:exc:`AttributeError` instead of logging the original failure

		assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE


		def test_serve_unservicable_logs_errors_without_crashing(mocker, capsys):

Uh oh!

Conversation

Tatamis commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What kind of change does this PR introduce?

What do these changes do?

Are there changes in behavior for the user?

Is it a substantial burden for the maintainers to support this?

Related issue number

Checklist

Test run

Uh oh!

read-the-docs-community Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

codecov Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

julianz- commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tatamis commented Sep 18, 2026

Uh oh!

julianz- commented Sep 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tatamis commented Sep 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tatamis commented Sep 17, 2026 •

edited

Loading

read-the-docs-community Bot commented Sep 17, 2026 •

edited

Loading

codecov Bot commented Sep 17, 2026 •

edited

Loading