Shut down task dispatcher's worker threads in BaseWSGIServer.close() - #497
Closed
agu2347 wants to merge 1 commit into
Closed
Shut down task dispatcher's worker threads in BaseWSGIServer.close()#497agu2347 wants to merge 1 commit into
agu2347 wants to merge 1 commit into
Conversation
MultiSocketServer.close() already shuts down its task dispatcher before closing sockets, but BaseWSGIServer.close() did not -- an inconsistency between two classes that both own/share a task_dispatcher. This matters more than it might first appear: for the common single-listen-address case, create_server() returns a bare BaseWSGIServer directly (not wrapped in MultiSocketServer, per its own "we can just return the last server" optimization), so calling .close() on it is the *only* place a caller has to shut things down without also calling .run() first. Without this fix, doing so left the dispatcher's worker (daemon) threads running. BaseWSGIServer.run() already calls task_dispatcher.shutdown() in its own SystemExit/KeyboardInterrupt handler, but that's a separate code path that only triggers if .run() was actually called and interrupted -- it doesn't help a caller who creates a server, then closes it directly (e.g. after a socket-binding failure elsewhere, or in test teardown, as described in the issue). Verified directly: created a real server with several worker threads, confirmed close() previously left those threads running (observable via threading.active_count()), and confirmed the fix correctly stops them. Also verified the multi-listen-address case (MultiSocketServer, which shares one task_dispatcher across multiple wrapped BaseWSGIServer instances) continues to work correctly with no double-shutdown issues, since ThreadedTaskDispatcher.shutdown() is idempotent (a no-op if no threads remain). Added a direct regression test for BaseWSGIServer.close() specifically (using the same DummyTaskDispatcher mock and was_shutdown flag pattern the existing, already-passing test_run test uses for the separate .run()-triggered shutdown path). Confirmed the new test fails with the original code and passes with the fix. Ran the full existing test_server.py suite (32 passed: 31 baseline + 1 new) and the broader project test suite (752 passed, 50 skipped; the 3 remaining failures are pre-existing socket-binding tests unrelated to this change). Fixes Pylons#480
Member
|
This looks like it's LLM generated, which is a red flag. I need to consult with others first and also check that you're not engaging in reputation laundering. |
Member
|
I've fixed this, alongside a bunch of other shutdown issues in this PR instead: #499 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #480.
MultiSocketServer.close()already shuts down its task dispatcher before closing sockets, butBaseWSGIServer.close()did not -- an inconsistency between two classes that both own/share atask_dispatcher. This matters more than it might first appear: for the common single-listen-address case,create_server()returns a bareBaseWSGIServerdirectly (not wrapped inMultiSocketServer, per its own "we can just return the last server" optimization), so calling.close()on it is the only place a caller has to shut things down without also calling.run()first. Without this fix, doing so left the dispatcher's worker (daemon) threads running.BaseWSGIServer.run()already callstask_dispatcher.shutdown()in its ownSystemExit/KeyboardInterrupthandler, but that's a separate code path that only triggers if.run()was actually called and interrupted -- it doesn't help a caller who creates a server, then closes it directly (e.g. after a socket-binding failure elsewhere, or in test teardown, as described in the issue).Testing: verified directly: created a real server with several worker threads, confirmed
close()previously left those threads running (observable viathreading.active_count()), and confirmed the fix correctly stops them. Also verified the multi-listen-address case (MultiSocketServer, which shares onetask_dispatcheracross multiple wrappedBaseWSGIServerinstances) continues to work correctly with no double-shutdown issues, sinceThreadedTaskDispatcher.shutdown()is idempotent (a no-op if no threads remain).Added a direct regression test for
BaseWSGIServer.close()specifically (using the sameDummyTaskDispatchermock andwas_shutdownflag pattern the existing, already-passingtest_runtest uses for the separate.run()-triggered shutdown path). I confirmed the new test fails with the original code and passes with the fix. Ran the full existingtest_server.pysuite (32 passed: 31 baseline + 1 new) and the broader project test suite (752 passed, 50 skipped; the 3 remaining failures are pre-existing socket-binding tests unrelated to this change).