Skip to content

Conversation

@itamarst
Copy link

@itamarst itamarst commented Nov 6, 2025

Implementing correct shutdown() would involve copying too much code, so switch to the more traditional way of indicating shutdown to a thread reading from a Queue.

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 #769
Superseded and closes #778

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

The overload thread leaks in Python < 3.13.

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

The thread no longer leaks.

📋 Other information:

📋 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

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.73%. Comparing base (22bb430) to head (8d1e5a0).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   79.60%   79.73%   +0.12%     
==========================================
  Files          29       30       +1     
  Lines        4261     4268       +7     
  Branches      542      543       +1     
==========================================
+ Hits         3392     3403      +11     
+ Misses        726      722       -4     
  Partials      143      143              

@itamarst itamarst marked this pull request as ready for review November 6, 2025 15:50
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itamarst thanks for looking into this! Too bad there's no backport for shutdown queues.

I have a few cosmetic requests but overall looks good. Could you tweak those? Thanks again!

Copy link

@nijel nijel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests in translate-toolkit pass with this applied, so for us this fixes #769.

@itamarst
Copy link
Author

itamarst commented Nov 7, 2025

OK, done.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Implementing correct shutdown() would involve copying too much code, so switch
to the more traditional way of indicating shutdown to a thread reading from a
Queue.

Signed-off-by: Itamar Turner-Trauring <[email protected]>
@itamarst
Copy link
Author

itamarst commented Nov 7, 2025

And I fixed the spelling issue too.

@webknjaz webknjaz merged commit bb698cf into cherrypy:main Nov 7, 2025
69 of 76 checks passed
@webknjaz
Copy link
Member

webknjaz commented Nov 7, 2025

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should the new _serve_unservicable thread be daemon?

4 participants