From 69d9374d3e7c41f15069ce1a80f9768d69af69e1 Mon Sep 17 00:00:00 2001 From: Hayao Date: Wed, 28 Jan 2026 20:33:39 +0900 Subject: [PATCH 1/3] Fix race condition by waiting for server ready state on startup --- pytest_httpserver/httpserver.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pytest_httpserver/httpserver.py b/pytest_httpserver/httpserver.py index 5e7f09d..ddee167 100644 --- a/pytest_httpserver/httpserver.py +++ b/pytest_httpserver/httpserver.py @@ -653,6 +653,7 @@ def __init__( self.ssl_context = ssl_context self.threaded = threaded self.no_handler_status_code = 500 + self._server_ready_event: threading.Event = threading.Event() def __repr__(self) -> str: return f"<{self.__class__.__name__} host={self.host} port={self.port}>" @@ -733,8 +734,12 @@ def thread_target(self) -> None: This method serves as a thread target when the server is started. This should not be called directly, but can be overridden to tailor it to your needs. + + If overriding, you must call ``self._server_ready_event.set()`` before starting + to serve requests, otherwise :py:meth:`start` will raise an error after timeout. """ assert self.server is not None + self._server_ready_event.set() self.server.serve_forever() def is_running(self) -> bool: @@ -774,8 +779,22 @@ def start(self) -> None: self.port = self.server.port # Update port (needed if `port` was set to 0) # Explicitly make the new thread daemonic to avoid shutdown issues + self._server_ready_event.clear() self.server_thread = threading.Thread(target=self.thread_target, daemon=True) self.server_thread.start() + if not self._server_ready_event.wait(timeout=10): + # Clean up the server before raising. + # Use server_close() instead of shutdown() to avoid deadlock + # if serve_forever() was never called. + self.server.server_close() + self.server_thread.join(timeout=5) + self.server = None + self.server_thread = None + raise HTTPServerError( + "Server did not start within timeout. " + "If you override thread_target(), ensure it calls " + "self._server_ready_event.set() before serving." + ) def stop(self) -> None: """ From 51afae6431c4eba175138cd68941dc85491035c4 Mon Sep 17 00:00:00 2001 From: Hayao Date: Wed, 28 Jan 2026 20:41:59 +0900 Subject: [PATCH 2/3] Add tests for server ready state fix --- pytest_httpserver/blocking_httpserver.py | 6 +- pytest_httpserver/httpserver.py | 61 ++++++++---- tests/test_release.py | 1 + tests/test_server_startup.py | 114 +++++++++++++++++++++++ 4 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 tests/test_server_startup.py diff --git a/pytest_httpserver/blocking_httpserver.py b/pytest_httpserver/blocking_httpserver.py index 096e9e4..cf74757 100644 --- a/pytest_httpserver/blocking_httpserver.py +++ b/pytest_httpserver/blocking_httpserver.py @@ -46,6 +46,8 @@ class BlockingHTTPServer(HTTPServerBase): :param timeout: waiting time in seconds for matching and responding to an incoming request. manager + :param startup_timeout: maximum time in seconds to wait for server readiness. + Set to ``None`` to disable readiness waiting. .. py:attribute:: no_handler_status_code @@ -63,8 +65,10 @@ def __init__( port: int = DEFAULT_LISTEN_PORT, ssl_context: SSLContext | None = None, timeout: int = 30, + *, + startup_timeout: float | None = 10.0, ) -> None: - super().__init__(host, port, ssl_context) + super().__init__(host, port, ssl_context, startup_timeout=startup_timeout) self.timeout = timeout self.request_queue: Queue[Request] = Queue() self.request_handlers: dict[Request, Queue[BlockingRequestHandler]] = {} diff --git a/pytest_httpserver/httpserver.py b/pytest_httpserver/httpserver.py index ddee167..b08894e 100644 --- a/pytest_httpserver/httpserver.py +++ b/pytest_httpserver/httpserver.py @@ -8,6 +8,7 @@ import threading import time import urllib.parse +import warnings from collections import defaultdict from collections.abc import Callable from collections.abc import Generator @@ -615,6 +616,8 @@ class HTTPServerBase(abc.ABC): # pylint: disable=too-many-instance-attributes :param port: the TCP port where the server will listen :param ssl_context: the ssl context object to use for https connections :param threaded: whether to handle concurrent requests in separate threads + :param startup_timeout: maximum time in seconds to wait for server readiness. + Set to ``None`` to disable readiness waiting. .. py:attribute:: log @@ -638,6 +641,7 @@ def __init__( ssl_context: SSLContext | None = None, *, threaded: bool = False, + startup_timeout: float | None = 10.0, ) -> None: """ Initializes the instance. @@ -652,6 +656,7 @@ def __init__( self.log: list[tuple[Request, Response]] = [] self.ssl_context = ssl_context self.threaded = threaded + self.startup_timeout = startup_timeout self.no_handler_status_code = 500 self._server_ready_event: threading.Event = threading.Event() @@ -735,8 +740,10 @@ def thread_target(self) -> None: This should not be called directly, but can be overridden to tailor it to your needs. - If overriding, you must call ``self._server_ready_event.set()`` before starting - to serve requests, otherwise :py:meth:`start` will raise an error after timeout. + If overriding, you should call ``self._server_ready_event.set()`` before starting + to serve requests. If the event is not set within the timeout, :py:meth:`start` + will emit a warning if the thread is still alive; if the thread dies during + startup, :py:meth:`start` raises an error. """ assert self.server is not None self._server_ready_event.set() @@ -779,22 +786,33 @@ def start(self) -> None: self.port = self.server.port # Update port (needed if `port` was set to 0) # Explicitly make the new thread daemonic to avoid shutdown issues - self._server_ready_event.clear() + # Create a new event for each startup to prevent stale threads from + # signaling readiness for a subsequent start() attempt. + self._server_ready_event = threading.Event() self.server_thread = threading.Thread(target=self.thread_target, daemon=True) self.server_thread.start() - if not self._server_ready_event.wait(timeout=10): - # Clean up the server before raising. - # Use server_close() instead of shutdown() to avoid deadlock - # if serve_forever() was never called. - self.server.server_close() - self.server_thread.join(timeout=5) - self.server = None - self.server_thread = None - raise HTTPServerError( - "Server did not start within timeout. " - "If you override thread_target(), ensure it calls " - "self._server_ready_event.set() before serving." - ) + if self.startup_timeout is not None and not self._server_ready_event.wait(timeout=self.startup_timeout): + # Event was not set within timeout. + # Check if thread is still alive (custom thread_target may not set the event) + if self.server_thread.is_alive(): + # Server thread is running, assume it's working (backward compatibility) + warnings.warn( + "Server thread is running but ready event was not set. " + "If you override thread_target(), call self._server_ready_event.set() " + "before serving to ensure reliable startup.", + stacklevel=2, + ) + else: + # Thread died, clean up and raise + self.server.server_close() + self.server_thread.join(timeout=5) + self.server = None + self.server_thread = None + raise HTTPServerError( + "Server thread died during startup. " + "If you override thread_target(), ensure it calls " + "self._server_ready_event.set() before serving." + ) def stop(self) -> None: """ @@ -956,6 +974,8 @@ class HTTPServer(HTTPServerBase): # pylint: disable=too-many-instance-attribute manager :param threaded: whether to handle concurrent requests in separate threads + :param startup_timeout: maximum time in seconds to wait for server readiness. + Set to ``None`` to disable readiness waiting. .. py:attribute:: no_handler_status_code @@ -975,11 +995,18 @@ def __init__( default_waiting_settings: WaitingSettings | None = None, *, threaded: bool = False, + startup_timeout: float | None = 10.0, ) -> None: """ Initializes the instance. """ - super().__init__(host, port, ssl_context, threaded=threaded) + super().__init__( + host, + port, + ssl_context, + threaded=threaded, + startup_timeout=startup_timeout, + ) self.ordered_handlers: list[RequestHandler] = [] self.oneshot_handlers = RequestHandlerList() diff --git a/tests/test_release.py b/tests/test_release.py index 5c3dbfd..caa1ac8 100644 --- a/tests/test_release.py +++ b/tests/test_release.py @@ -232,6 +232,7 @@ def test_sdist_contents(build: Build, version: str): "test_querymatcher.py", "test_querystring.py", "test_release.py", + "test_server_startup.py", "test_ssl.py", "test_thread_type.py", "test_threaded.py", diff --git a/tests/test_server_startup.py b/tests/test_server_startup.py new file mode 100644 index 0000000..a6f324e --- /dev/null +++ b/tests/test_server_startup.py @@ -0,0 +1,114 @@ +from __future__ import annotations + +import contextlib +import socket +import time + +import pytest + +from pytest_httpserver import HTTPServer + + +def test_server_ready_immediately_after_start(): + """Test that the server accepts connections immediately after start() returns.""" + server = HTTPServer(host="localhost", port=0) + server.expect_request("/").respond_with_data("ok") + server.start() + try: + # Attempt to connect immediately - should not fail + sock = socket.create_connection((server.host, server.port), timeout=1) + sock.close() + finally: + server.stop() + + +def test_server_ready_under_load(): + """Test that the server is ready even when started multiple times in succession.""" + for _ in range(10): + server = HTTPServer(host="localhost", port=0) + server.expect_request("/").respond_with_data("ok") + server.start() + try: + sock = socket.create_connection((server.host, server.port), timeout=1) + sock.close() + finally: + server.stop() + + +class SlowStartServer(HTTPServer): + """A server subclass that simulates slow startup.""" + + def thread_target(self): + time.sleep(0.5) # Simulate slow initialization + self._server_ready_event.set() + assert self.server is not None + self.server.serve_forever() + + +class NoReadyEventServer(HTTPServer): + """A server subclass that never signals readiness.""" + + def thread_target(self): + assert self.server is not None + self.server.serve_forever() + + +def test_slow_start_server_waits_for_ready(): + """Test that start() waits for slow thread_target implementations.""" + server = SlowStartServer(host="localhost", port=0) + server.expect_request("/").respond_with_data("ok") + + start_time = time.time() + server.start() + elapsed = time.time() - start_time + + try: + # Should have waited at least 0.5 seconds + assert elapsed >= 0.5 + # Server should be ready + sock = socket.create_connection((server.host, server.port), timeout=1) + sock.close() + finally: + server.stop() + + +def test_new_event_created_for_each_start(): + """Test that a new event is created for each start() to isolate retries.""" + server = HTTPServer(host="localhost", port=0) + server.expect_request("/").respond_with_data("ok") + + original_event = server._server_ready_event # noqa: SLF001 + + server.start() + first_start_event = server._server_ready_event # noqa: SLF001 + server.stop() + + server.start() + second_start_event = server._server_ready_event # noqa: SLF001 + server.stop() + + # Each start() should create a new event + assert first_start_event is not original_event + assert second_start_event is not first_start_event + + +def test_warns_when_ready_event_not_set(): + """Test that a warning is emitted when the ready event is never set.""" + server = NoReadyEventServer(host="localhost", port=0, startup_timeout=0.0) + server.expect_request("/").respond_with_data("ok") + + with pytest.warns(UserWarning, match="ready event was not set"): + server.start() + + try: + deadline = time.time() + 1 + while time.time() < deadline: + with contextlib.suppress(OSError): + sock = socket.create_connection((server.host, server.port), timeout=0.1) + sock.close() + break + time.sleep(0.01) + else: + raise AssertionError("Server did not accept connections within 1 second") + finally: + server.stop() From 0a3e6ac00ea5d477740a55c216911fc812cd6496 Mon Sep 17 00:00:00 2001 From: Hayao Date: Fri, 30 Jan 2026 17:40:53 +0900 Subject: [PATCH 3/3] Add more tests --- tests/test_server_startup.py | 92 ++++++++++++++++++++++++++++++++---- 1 file changed, 83 insertions(+), 9 deletions(-) diff --git a/tests/test_server_startup.py b/tests/test_server_startup.py index a6f324e..adf8647 100644 --- a/tests/test_server_startup.py +++ b/tests/test_server_startup.py @@ -5,11 +5,13 @@ import time import pytest +import requests +from requests.exceptions import Timeout from pytest_httpserver import HTTPServer -def test_server_ready_immediately_after_start(): +def test_server_ready_immediately_after_start() -> None: """Test that the server accepts connections immediately after start() returns.""" server = HTTPServer(host="localhost", port=0) server.expect_request("/").respond_with_data("ok") @@ -22,7 +24,7 @@ def test_server_ready_immediately_after_start(): server.stop() -def test_server_ready_under_load(): +def test_server_ready_under_load() -> None: """Test that the server is ready even when started multiple times in succession.""" for _ in range(10): server = HTTPServer(host="localhost", port=0) @@ -38,7 +40,7 @@ def test_server_ready_under_load(): class SlowStartServer(HTTPServer): """A server subclass that simulates slow startup.""" - def thread_target(self): + def thread_target(self) -> None: time.sleep(0.5) # Simulate slow initialization self._server_ready_event.set() assert self.server is not None @@ -48,19 +50,19 @@ def thread_target(self): class NoReadyEventServer(HTTPServer): """A server subclass that never signals readiness.""" - def thread_target(self): + def thread_target(self) -> None: assert self.server is not None self.server.serve_forever() -def test_slow_start_server_waits_for_ready(): +def test_slow_start_server_waits_for_ready() -> None: """Test that start() waits for slow thread_target implementations.""" server = SlowStartServer(host="localhost", port=0) server.expect_request("/").respond_with_data("ok") - start_time = time.time() + start_time = time.monotonic() server.start() - elapsed = time.time() - start_time + elapsed = time.monotonic() - start_time try: # Should have waited at least 0.5 seconds @@ -72,7 +74,7 @@ def test_slow_start_server_waits_for_ready(): server.stop() -def test_new_event_created_for_each_start(): +def test_new_event_created_for_each_start() -> None: """Test that a new event is created for each start() to isolate retries.""" server = HTTPServer(host="localhost", port=0) server.expect_request("/").respond_with_data("ok") @@ -92,7 +94,7 @@ def test_new_event_created_for_each_start(): assert second_start_event is not first_start_event -def test_warns_when_ready_event_not_set(): +def test_warns_when_ready_event_not_set() -> None: """Test that a warning is emitted when the ready event is never set.""" server = NoReadyEventServer(host="localhost", port=0, startup_timeout=0.0) server.expect_request("/").respond_with_data("ok") @@ -112,3 +114,75 @@ def test_warns_when_ready_event_not_set(): raise AssertionError("Server did not accept connections within 1 second") finally: server.stop() + + +class SlowServeServer(HTTPServer): + """A server that delays serve_forever() but does not set ready event early. + + This simulates the scenario where: + - bind() and listen() complete (TCP connections queue in backlog) + - But serve_forever() hasn't started yet (no HTTP responses) + """ + + def thread_target(self) -> None: + assert self.server is not None + # Delay before serve_forever - connections will queue but not be processed + time.sleep(3.0) + self._server_ready_event.set() + self.server.serve_forever() + + +def test_http_request_fails_before_serve_forever_without_wait() -> None: + """ + Demonstrate the race condition: TCP connects but HTTP times out. + + This test shows why waiting for server readiness matters: + - After start(), TCP connections succeed (queued in backlog) + - But HTTP requests timeout because serve_forever() hasn't started + - With short client timeouts (common in production), this causes failures + """ + # Use startup_timeout=0 to NOT wait for ready event (old behavior) + server = SlowServeServer(host="localhost", port=0, startup_timeout=0.0) + server.expect_request("/ping").respond_with_data("pong") + + with pytest.warns(UserWarning, match="ready event was not set"): + server.start() + + try: + # TCP connection succeeds (proves Zsolt's point about backlog) + sock = socket.create_connection((server.host, server.port), timeout=1) + sock.close() + + # But HTTP request with short timeout fails! + # This is the actual problem in containerized environments + with pytest.raises(Timeout): + requests.get(server.url_for("/ping"), timeout=(0.5, 0.5)) + finally: + server.stop() + + +def test_http_request_succeeds_when_waiting_for_ready() -> None: + """ + Demonstrate that waiting for ready event fixes the race condition. + + With startup_timeout enabled (default), start() waits until + serve_forever() begins, so HTTP requests succeed immediately. + """ + # Use default startup_timeout to wait for ready event + server = SlowServeServer(host="localhost", port=0) # default startup_timeout=10.0 + server.expect_request("/ping").respond_with_data("pong") + + start_time = time.monotonic() + server.start() + elapsed = time.monotonic() - start_time + + try: + # Should have waited for the slow startup + assert elapsed >= 3.0, f"Expected to wait >= 3.0s, but only waited {elapsed}s" + + # HTTP request succeeds because serve_forever() has started + response = requests.get(server.url_for("/ping"), timeout=(0.5, 0.5)) + assert response.status_code == 200 + assert response.text == "pong" + finally: + server.stop()