From 105aa83cd260083e760f927877d306442eca294a Mon Sep 17 00:00:00 2001 From: agu2347 Date: Tue, 28 Jul 2026 11:18:01 +0000 Subject: [PATCH] Shut down task dispatcher's worker threads in BaseWSGIServer.close() 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 #480 --- CHANGES.txt | 8 ++++++++ src/waitress/server.py | 9 +++++++++ tests/test_server.py | 19 +++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index c7f32ea4..f90eae6f 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -4,6 +4,14 @@ Unreleased Bugfix ~~~~~~ +- ``BaseWSGIServer.close()`` now shuts down the task dispatcher's worker + threads, matching ``MultiSocketServer.close()``'s existing behavior. This + matters most for the common single-listen-address case, where + ``create_server()`` returns a bare ``BaseWSGIServer`` (not wrapped in + ``MultiSocketServer``), so calling ``.close()`` directly used to leave the + dispatcher's worker threads running. See + https://github.com/Pylons/waitress/issues/480 + - Renamed the HTTP header "Trailers" to "Trailer" to fix a typo and comply with the correct header name as specified in RFC 7230. diff --git a/src/waitress/server.py b/src/waitress/server.py index c56530a0..73b40a10 100644 --- a/src/waitress/server.py +++ b/src/waitress/server.py @@ -355,6 +355,15 @@ def print_listen(self, format_str): # pragma: no cover def close(self): self.trigger.close() + # Ensure the task dispatcher's worker threads are stopped too, + # matching MultiSocketServer.close()'s existing behavior. This + # matters most for the common single-listen-address case, + # where create_server() returns a bare BaseWSGIServer (not + # wrapped in MultiSocketServer), so calling .close() on it is + # the only place a caller has to shut things down -- without + # this, the dispatcher's worker threads were left running + # after close(). See GH #480. + self.task_dispatcher.shutdown() return wasyncore.dispatcher.close(self) diff --git a/tests/test_server.py b/tests/test_server.py index cede49a7..da3c8d0b 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -124,6 +124,25 @@ def test_run(self): inst.run() self.assertTrue(inst.task_dispatcher.was_shutdown) + def test_close_shuts_down_task_dispatcher(self): + """ + Regression test for + https://github.com/Pylons/waitress/issues/480 + + close() must shut down the task dispatcher's worker threads + directly, not only as a side effect of run()'s + SystemExit/KeyboardInterrupt handling. This matters most for + the common single-listen-address case, where create_server() + returns a bare BaseWSGIServer (not wrapped in + MultiSocketServer), so calling .close() directly -- without + ever calling .run() -- is the only place a caller has to shut + things down. + """ + inst = self._makeOneWithMap(_start=False) + inst.task_dispatcher = DummyTaskDispatcher() + inst.close() + self.assertTrue(inst.task_dispatcher.was_shutdown) + def test_run_base_server(self): inst = self._makeOneWithMulti(_start=False) inst.asyncore = DummyAsyncore()