Skip to content

Fix unservicable connection error logging#830

Open
andrewkernel wants to merge 3 commits into
cherrypy:mainfrom
andrewkernel:codex/fix-unservicable-error-log
Open

Fix unservicable connection error logging#830
andrewkernel wants to merge 3 commits into
cherrypy:mainfrom
andrewkernel:codex/fix-unservicable-error-log

Conversation

@andrewkernel

Copy link
Copy Markdown

What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • 📋 docs update
  • 📋 tests/coverage improvement
  • 📋 refactoring
  • 💥 other

📋 What is the related issue number (starting with #)

Resolves #797

What is the current behavior?

HTTPServer._serve_unservicable() calls self.server.error_log() when HTTPRequest.simple_response() raises an unexpected exception, but HTTPServer does not have a server attribute. That means the 503 handler's error logging path raises another exception instead of logging the original failure.

What is the new behavior?

The 503 handler now calls self.error_log(), matching other HTTPServer logging paths. A focused regression test covers the unexpected-error branch and verifies that the connection is still marked for linger/close.

📋 Other information:

Verification:

  • python -m py_compile cheroot/server.py cheroot/test/test_server.py
  • .\.venv\Scripts\python.exe -m pytest cheroot/test/test_server.py -k unservicable -q --no-cov
  • .\.venv\Scripts\python.exe -m pytest cheroot/test/test_server.py::test_stop_interrupts_serve cheroot/test/test_server.py::test_unservicable_conn_logs_unexpected_response_errors -q --no-cov

The targeted pytest selection also passes under the repository config, but the repo-wide coverage threshold fails when only this single test is selected; the --no-cov runs above verify the regression locally.

📋 Contribution checklist:

  • I wrote descriptive pull request text above
  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences

@read-the-docs-community

read-the-docs-community Bot commented Jun 25, 2026

Copy link
Copy Markdown

Documentation build overview

📚 cheroot | 🛠️ Build #33300654 | 📁 Comparing b0177b8 against latest (3937fe1)

  🔍 Preview build  

6 files changed · ± 6 modified

± Modified

@andrewkernel andrewkernel force-pushed the codex/fix-unservicable-error-log branch from 60b82ae to d179d6e Compare June 25, 2026 03:25
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.33%. Comparing base (3937fe1) to head (b0177b8).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #830      +/-   ##
==========================================
+ Coverage   78.09%   78.33%   +0.24%     
==========================================
  Files          41       41              
  Lines        4788     4810      +22     
  Branches      547      547              
==========================================
+ Hits         3739     3768      +29     
+ Misses        910      903       -7     
  Partials      139      139              

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant