diff --git a/cheroot/server.py b/cheroot/server.py index 284cf17c72..1a0970076d 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -1904,7 +1904,7 @@ def _serve_unservicable(self): # We can't just raise an exception because that will kill this # thread, and prevent 503 errors from being sent to future # connections. - self.server.error_log( + self.error_log( repr(ex), level=logging.ERROR, traceback=True, diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index ae6e390a44..23a044bf6c 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -9,6 +9,7 @@ import tempfile import threading import types +import typing as _t import urllib.parse # noqa: WPS301 import uuid from http import HTTPStatus @@ -20,7 +21,7 @@ from pypytools.gc.custom import DefaultGc from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM, bton, ntob -from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPServer +from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPRequest, HTTPServer from ..testing import ( ANY_INTERFACE_IPV4, ANY_INTERFACE_IPV6, @@ -583,11 +584,11 @@ def test_threadpool_multistart_validation(monkeypatch): tp.start() -def test_overload_results_in_suitable_http_error(request): - """A server that can't keep up with requests returns a 503 HTTP error.""" - localhost = '127.0.0.1' +@pytest.fixture +def overloaded_http_server() -> _t.Iterator[HTTPServer]: + """Return a running server that answers every request with a 503.""" httpserver = HTTPServer( - bind_addr=(localhost, EPHEMERAL_PORT), + bind_addr=('127.0.0.1', EPHEMERAL_PORT), gateway=Gateway, ) # Can only handle on request in parallel: @@ -602,19 +603,76 @@ def test_overload_results_in_suitable_http_error(request): httpserver.prepare() serve_thread = threading.Thread(target=httpserver.serve) serve_thread.start() - request.addfinalizer(httpserver.stop) - # Stop the thread pool to ensure the queue fills up: - httpserver.requests.stop() + try: + # Stop the thread pool to ensure the queue fills up: + httpserver.requests.stop() + + # Use up the very limited thread pool queue we've set up, so future + # requests fail: + httpserver.requests._queue.put(None) + + yield httpserver + finally: + httpserver.stop() + + +def test_overload_results_in_suitable_http_error( + overloaded_http_server: HTTPServer, +) -> None: + """A server that can't keep up with requests returns a 503 HTTP error.""" + host, port = overloaded_http_server.bind_addr + + response = requests.get(f'http://{host}:{port}', timeout=20) + assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE + + +def test_overload_survives_failure_to_send_http_error( + overloaded_http_server: HTTPServer, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """An unexpected error while sending a 503 is logged, not fatal. + + The overload handling must keep on answering later connections. + + This is a test for issue #797. + """ + host, port = overloaded_http_server.bind_addr + real_simple_response = HTTPRequest.simple_response + attempts = [] + + def failing_once_simple_response( + request: HTTPRequest, + status: str, + msg: str = '', + ) -> None: + attempts.append(status) + if len(attempts) == 1: + raise RuntimeError('unexpected failure sending the 503') + real_simple_response(request, status, msg) - _host, port = httpserver.bind_addr + monkeypatch.setattr( + HTTPRequest, + 'simple_response', + failing_once_simple_response, + ) - # Use up the very limited thread pool queue we've set up, so future - # requests fail: - httpserver.requests._queue.put(None) + # The server fails to send the 503 to this connection and leaves it + # lingering, so the client won't ever get a response over it: + with socket.create_connection((host, port), timeout=20): + # The overload thread handles connections one by one, so once this + # one is answered, the failed attempt above has been dealt with: + response = requests.get(f'http://{host}:{port}', timeout=20) - response = requests.get(f'http://{localhost}:{port}', timeout=20) assert response.status_code == HTTPStatus.SERVICE_UNAVAILABLE + # The original failure is reported instead of a bogus `AttributeError`: + captured_stderr = capsys.readouterr().err + assert ( + 'RuntimeError: unexpected failure sending the 503' in captured_stderr + ) + assert 'AttributeError' not in captured_stderr + def test_overload_thread_does_not_leak(): """On shutdown the overload thread exits. diff --git a/docs/changelog-fragments.d/797.bugfix.rst b/docs/changelog-fragments.d/797.bugfix.rst new file mode 100644 index 0000000000..4ff21f45e0 --- /dev/null +++ b/docs/changelog-fragments.d/797.bugfix.rst @@ -0,0 +1,7 @@ +Fixed the server's ability to send ``503 Service Unavailable`` whenever the +server becomes too busy. Failures to send caused the handler for this message +to crash, leaving new connections during busy times unanswered. Any failure in +sending this message is now logged, and subsequent connections continue to be +answered. + +-- by :user:`Tatamis`, :user:`avinashkamat48` and :user:`andrewkernel` diff --git a/docs/conf.py b/docs/conf.py index 1d526cae44..e97a905e73 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -150,6 +150,10 @@ ('py:class', '_pyio.BufferedWriter'), ('py:class', '_pyio.BufferedReader'), ('py:class', 'unittest.case.TestCase'), + # Pytest exposes these under the public `pytest.` names but the + # documented objects live in its private `_pytest` package: + ('py:class', '_pytest.capture.CaptureFixture'), + ('py:class', '_pytest.monkeypatch.MonkeyPatch'), ('py:meth', 'cheroot.connections.ConnectionManager.get_conn'), # Ref: https://github.com/pyca/pyopenssl/issues/1012 ('py:class', 'pyopenssl:OpenSSL.SSL.Context'),