Skip to content

Daemonize the serve_unservicable thread#778

Closed
manthey wants to merge 1 commit intocherrypy:mainfrom
manthey:daemonize-unservicable
Closed

Daemonize the serve_unservicable thread#778
manthey wants to merge 1 commit intocherrypy:mainfrom
manthey:daemonize-unservicable

Conversation

@manthey
Copy link
Copy Markdown

@manthey manthey commented Oct 2, 2025

In Python < 3.13, the code backfills a shutdown method for a queue, but this isn't complete; an existing get call is not interrupted by shutdown and this means that when the unservicable queue is empty, it never stops, preventing the system from stopping. By daemonizing the unservicable thread, the server can stop and exit.

A better solution would probably involve copying a fair amount of code from Python 3.13's queue implementation. In my opinion that isn't worth the maintenance burden. If desired, perhaps we should only daemonize this thread on Python < 3.13.

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

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

It is impossible to stop a server that has no pending unservicable requests on Python < 3.13.

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

You can stop a server that has no pending unservicable requests.


This change is Reviewable

In Python < 3.13, the code backfills a shutdown method for a queue, but
this isn't complete; an existing get call is not interrupted by shutdown
and this means that when the unservicable queue is empty, it never
stops, preventing the system from stopping.  By daemonizing the
unservicable thread, the server can stop and exit.

A better solution would probably involve copying a fair amount of code
from Python 3.13's queue implementation.  In my opinion that isn't worth
the maintenance burden.  If desired, perhaps we should only daemonize
this thread on Python < 3.13.

Signed-off-by: David Manthey <david.manthey@kitware.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.24%. Comparing base (4f040de) to head (505a48e).
⚠️ Report is 160 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
- Coverage   79.31%   79.24%   -0.08%     
==========================================
  Files          29       29              
  Lines        4197     4197              
  Branches      538      538              
==========================================
- Hits         3329     3326       -3     
- Misses        726      727       +1     
- Partials      142      144       +2     

@webknjaz webknjaz added bug Something is broken regression Something that worked earlier got broken in new release labels Oct 3, 2025
Copy link
Copy Markdown
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 since you submitted the original PR, may I ask you to check this bug fix?

@manthey could you add a bugfix change note and maybe a regression test? https://cheroot.cherrypy.dev/en/latest/contributing/guidelines/#adding-change-notes-with-your-prs

@itamarst
Copy link
Copy Markdown

itamarst commented Oct 3, 2025

I will try to look at it on Monday, if I don't do it then, remind me again.

@martin-vi
Copy link
Copy Markdown

martin-vi commented Oct 7, 2025

Sorry but daemon=True does not solve the issue here for Python 3.12. In particular, when cheroot is used in test cases, more and more threads accumulate.

Here is a somewhat simple test case to illustrate the problem. It is best to run it without xdist to see it better:

diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py
index fb5c5468..5c1251d6 100644
--- a/cheroot/test/test_server.py
+++ b/cheroot/test/test_server.py
@@ -611,3 +611,17 @@ def test_overload_results_in_suitable_http_error(request):

     response = requests.get(f'http://{localhost}:{port}', timeout=20)
     assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE
+
+
+def test_unservicable_threadleak():
+    before_serv = threading.active_count()
+    # assert before_serv == 1 # <- does not work with xdist ..
+    for _ in range(10):
+        httpserver = HTTPServer(
+                bind_addr=(ANY_INTERFACE_IPV4, EPHEMERAL_PORT),
+                gateway=Gateway,
+        )
+        with httpserver._run_in_thread() as server:
+            import time; time.sleep(0.5)
+
+    assert threading.active_count() == before_serv

Run pytest with --pdb and inspect threads with threading.enumerate() you will spot all the pilled up _serve_unservicable threads that are never tidied up.

@webknjaz
Copy link
Copy Markdown
Member

@manthey could you include Martin's test case, please?

@itamarst do you have more ideas on improving this?

@webknjaz
Copy link
Copy Markdown
Member

@itamarst hey, have you had a chance to lurk in?

@itamarst
Copy link
Copy Markdown

This seems fine, it's supposed to be a no side-effect add on.

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Nov 3, 2025

This seems fine, it's supposed to be a no side-effect add on.

@itamarst what about the comment above? Looks like the thread remains lingering in some contexts (including maybe cherrypy/cherrypy#2070).

@martin-vi you mentioned Python 3.12 — is that the only version you tested?

@itamarst
Copy link
Copy Markdown

itamarst commented Nov 3, 2025

OK I'll dig in to this Wednesday (tomorrow I'm doing local election volunteering).

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Nov 5, 2025

@itamarst okay, thanks

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Nov 6, 2025

UPD: @woodruffw reported that this affects psf/cachecontrol#392 too.

@itamarst
Copy link
Copy Markdown

itamarst commented Nov 6, 2025

Wednesday I was too sleep deprived to function, but looking at this now.

@itamarst
Copy link
Copy Markdown

itamarst commented Nov 6, 2025

I went for a different approach in #794. It includes a test based on the reproducer above.

@webknjaz
Copy link
Copy Markdown
Member

webknjaz commented Nov 7, 2025

🎉 v11.1.2 just hit PyPI: https://pypi.org/project/cheroot/11.1.2/

This should be fixed via #794 now.

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

Labels

bug Something is broken regression Something that worked earlier got broken in new release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should the new _serve_unservicable thread be daemon?

4 participants