Skip to content

🐛 Handle stray exceptions in worker threads#649

Merged
webknjaz merged 6 commits into
cherrypy:mainfrom
webknjaz:bugfixes/358-unhandled-worker-thread-exc
Mar 31, 2024
Merged

🐛 Handle stray exceptions in worker threads#649
webknjaz merged 6 commits into
cherrypy:mainfrom
webknjaz:bugfixes/358-unhandled-worker-thread-exc

Conversation

@webknjaz

@webknjaz webknjaz commented Mar 12, 2024

Copy link
Copy Markdown
Member

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 #)

Fixes #358
Ref #375
Resolves #365

What is the current behavior? (You can also link to an open issue here)

A DoS would happen in many situations, including TLS errors and attempts to close the underlying sockets erroring out.

What is the new behavior (if this is a feature change)?

This patch aims to prevent a situation when the working threads are killed by arbitrary exceptions that bubble up to their entry point layers that aren't handled properly or at all.

📋 Other information:

N/A

📋 Contribution checklist:

(If you're a first-timer, check out
this guide on making great pull requests)

  • 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

This change is Reviewable

@webknjaz webknjaz added bug Something is broken backport-8.x 🤖 Trigger automatic backporting into the `maint/8.x` release branch by the Patchback robot labels Mar 12, 2024
@webknjaz webknjaz self-assigned this Mar 12, 2024
@webknjaz
webknjaz force-pushed the bugfixes/358-unhandled-worker-thread-exc branch from d1f72d0 to a11d38d Compare March 13, 2024 10:39
@codecov

codecov Bot commented Mar 13, 2024

Copy link
Copy Markdown

Codecov Report

Merging #649 (a7665f3) into main (029600e) will increase coverage by 1.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #649      +/-   ##
==========================================
+ Coverage   82.57%   83.58%   +1.00%     
==========================================
  Files          28       28              
  Lines        4068     4172     +104     
==========================================
+ Hits         3359     3487     +128     
+ Misses        709      685      -24     

@webknjaz
webknjaz force-pushed the bugfixes/358-unhandled-worker-thread-exc branch from a11d38d to 432a855 Compare March 13, 2024 16:10
@webknjaz
webknjaz force-pushed the bugfixes/358-unhandled-worker-thread-exc branch 6 times, most recently from c3e8592 to aa64fe0 Compare March 24, 2024 00:03
This patch aims to prevent a situation when the working threads are
killed by arbitrary exceptions that bubble up to their entry point
layers that aren't handled properly or at all. This was causing DoS
in many situations, including TLS errors and attempts to close the
underlying sockets erroring out.

Fixes cherrypy#358
Ref cherrypy#375
Resolves cherrypy#365
This simulates a situation when the server attempts sending at least
something useful to a connection with broken HTTP.
This documents the expected keyboard interrupt server shutdown
handling sequence.
@webknjaz
webknjaz force-pushed the bugfixes/358-unhandled-worker-thread-exc branch 2 times, most recently from 56bad54 to abde13f Compare March 27, 2024 19:00
This is to ensure that such exceptions don't leak into the internal
worker thread logic.
This is to ensure that such exceptions don't crash the worker threads
starving the server of the processing resources.
This is necessary since tests are more prone to linting exceptions.
@webknjaz
webknjaz force-pushed the bugfixes/358-unhandled-worker-thread-exc branch from abde13f to a7665f3 Compare March 27, 2024 22:20
@webknjaz
webknjaz merged commit 9720c31 into cherrypy:main Mar 31, 2024
@patchback

This comment was marked as outdated.

webknjaz added a commit that referenced this pull request Apr 14, 2024
@webknjaz

Copy link
Copy Markdown
Member Author

UPD: this change was released as a part of v10.0.1: #365 (comment).

scyclops added a commit to scyclops/cheroot that referenced this pull request May 3, 2024
@PraveenKumarVN

Copy link
Copy Markdown

@webknjaz I tried with cheroot v10.0.1 , still seeing same issue mentioned in cherrypy/cherrypy#2025

@webknjaz

webknjaz commented May 9, 2024

Copy link
Copy Markdown
Member Author

That's weird. Could you file an issue with a Cheroot-only reproducer?

@PraveenKumarVN

Copy link
Copy Markdown

@webknjaz #709

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

Labels

backport-8.x 🤖 Trigger automatic backporting into the `maint/8.x` release branch by the Patchback robot bug Something is broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Threadpool worker threads exit on socket errors

2 participants