diff --git a/lib/cheroot/connections.py b/lib/cheroot/connections.py index 9346bc6a..df70e6ea 100644 --- a/lib/cheroot/connections.py +++ b/lib/cheroot/connections.py @@ -292,7 +292,20 @@ class ConnectionManager: if self.server.ssl_adapter is not None: try: s, ssl_env = self.server.ssl_adapter.wrap(s) - except errors.NoSSLError: + except errors.FatalSSLAlert as tls_connection_drop_error: + self.server.error_log( + f'Client {addr !s} lost โ€” peer dropped the TLS ' + 'connection suddenly, during handshake: ' + f'{tls_connection_drop_error !s}', + ) + return + except errors.NoSSLError as http_over_https_err: + self.server.error_log( + f'Client {addr !s} attempted to speak plain HTTP into ' + 'a TCP connection configured for TLS-only traffic โ€” ' + 'trying to send back a plain HTTP error response: ' + f'{http_over_https_err !s}', + ) msg = ( 'The client sent a plain HTTP request, but ' 'this server only speaks HTTPS on this port.' @@ -311,8 +324,6 @@ class ConnectionManager: if ex.args[0] not in errors.socket_errors_to_ignore: raise return - if not s: - return mf = self.server.ssl_adapter.makefile # Re-apply our timeout since we may have a new socket object if hasattr(s, 'settimeout'): diff --git a/lib/cheroot/server.py b/lib/cheroot/server.py index bceeb2c9..91564611 100644 --- a/lib/cheroot/server.py +++ b/lib/cheroot/server.py @@ -157,7 +157,7 @@ QUOTED_SLASH = b'%2F' QUOTED_SLASH_REGEX = re.compile(b''.join((b'(?i)', QUOTED_SLASH))) -_STOPPING_FOR_INTERRUPT = object() # sentinel used during shutdown +_STOPPING_FOR_INTERRUPT = Exception() # sentinel used during shutdown comma_separated_headers = [ @@ -209,7 +209,11 @@ class HeaderReader: if not line.endswith(CRLF): raise ValueError('HTTP requires CRLF terminators') - if line[0] in (SPACE, TAB): + if line[:1] in (SPACE, TAB): + # NOTE: `type(line[0]) is int` and `type(line[:1]) is bytes`. + # NOTE: The former causes a the following warning: + # NOTE: `BytesWarning('Comparison between bytes and int')` + # NOTE: The latter is equivalent and does not. # It's a continuation line. v = line.strip() else: @@ -1725,16 +1729,16 @@ class HTTPServer: """Run the server forever, and stop it cleanly on exit.""" try: self.start() - except (KeyboardInterrupt, IOError): - # The time.sleep call might raise - # "IOError: [Errno 4] Interrupted function call" on KBInt. - self.error_log('Keyboard Interrupt: shutting down') - self.stop() - raise - except SystemExit: - self.error_log('SystemExit raised: shutting down') - self.stop() - raise + except KeyboardInterrupt as kb_intr_exc: + underlying_interrupt = self.interrupt + if not underlying_interrupt: + self.interrupt = kb_intr_exc + raise kb_intr_exc from underlying_interrupt + except SystemExit as sys_exit_exc: + underlying_interrupt = self.interrupt + if not underlying_interrupt: + self.interrupt = sys_exit_exc + raise sys_exit_exc from underlying_interrupt def prepare(self): # noqa: C901 # FIXME """Prepare server to serving requests. @@ -2111,6 +2115,13 @@ class HTTPServer: has completed. """ self._interrupt = _STOPPING_FOR_INTERRUPT + + if isinstance(interrupt, KeyboardInterrupt): + self.error_log('Keyboard Interrupt: shutting down') + + if isinstance(interrupt, SystemExit): + self.error_log('SystemExit raised: shutting down') + self.stop() self._interrupt = interrupt diff --git a/lib/cheroot/ssl/builtin.py b/lib/cheroot/ssl/builtin.py index b22d4ae6..e28e5df1 100644 --- a/lib/cheroot/ssl/builtin.py +++ b/lib/cheroot/ssl/builtin.py @@ -27,12 +27,9 @@ except ImportError: from . import Adapter from .. import errors -from .._compat import IS_ABOVE_OPENSSL10 from ..makefile import StreamReader, StreamWriter from ..server import HTTPServer -generic_socket_error = OSError - def _assert_ssl_exc_contains(exc, *msgs): """Check whether SSL exception contains either of messages provided.""" @@ -265,62 +262,35 @@ class BuiltinSSLAdapter(Adapter): def wrap(self, sock): """Wrap and return the given socket, plus WSGI environ entries.""" - EMPTY_RESULT = None, {} try: s = self.context.wrap_socket( sock, do_handshake_on_connect=True, server_side=True, ) - except ssl.SSLError as ex: - if ex.errno == ssl.SSL_ERROR_EOF: - # This is almost certainly due to the cherrypy engine - # 'pinging' the socket to assert it's connectable; - # the 'ping' isn't SSL. - return EMPTY_RESULT - elif ex.errno == ssl.SSL_ERROR_SSL: - if _assert_ssl_exc_contains(ex, 'http request'): - # The client is speaking HTTP to an HTTPS server. - raise errors.NoSSLError + except ( + ssl.SSLEOFError, + ssl.SSLZeroReturnError, + ) as tls_connection_drop_error: + raise errors.FatalSSLAlert( + *tls_connection_drop_error.args, + ) from tls_connection_drop_error + except ssl.SSLError as generic_tls_error: + peer_speaks_plain_http_over_https = ( + generic_tls_error.errno == ssl.SSL_ERROR_SSL and + _assert_ssl_exc_contains(generic_tls_error, 'http request') + ) + if peer_speaks_plain_http_over_https: + reraised_connection_drop_exc_cls = errors.NoSSLError + else: + reraised_connection_drop_exc_cls = errors.FatalSSLAlert - # Check if it's one of the known errors - # Errors that are caught by PyOpenSSL, but thrown by - # built-in ssl - _block_errors = ( - 'unknown protocol', 'unknown ca', 'unknown_ca', - 'unknown error', - 'https proxy request', 'inappropriate fallback', - 'wrong version number', - 'no shared cipher', 'certificate unknown', - 'ccs received early', - 'certificate verify failed', # client cert w/o trusted CA - 'version too low', # caused by SSL3 connections - 'unsupported protocol', # caused by TLS1 connections - ) - if _assert_ssl_exc_contains(ex, *_block_errors): - # Accepted error, let's pass - return EMPTY_RESULT - elif _assert_ssl_exc_contains(ex, 'handshake operation timed out'): - # This error is thrown by builtin SSL after a timeout - # when client is speaking HTTP to an HTTPS server. - # The connection can safely be dropped. - return EMPTY_RESULT - raise - except generic_socket_error as exc: - """It is unclear why exactly this happens. + raise reraised_connection_drop_exc_cls( + *generic_tls_error.args, + ) from generic_tls_error + except OSError as tcp_connection_drop_error: + raise errors.FatalSSLAlert( + *tcp_connection_drop_error.args, + ) from tcp_connection_drop_error - It's reproducible only with openssl>1.0 and stdlib - :py:mod:`ssl` wrapper. - In CherryPy it's triggered by Checker plugin, which connects - to the app listening to the socket port in TLS mode via plain - HTTP during startup (from the same process). - - - Ref: https://github.com/cherrypy/cherrypy/issues/1618 - """ - is_error0 = exc.args == (0, 'Error') - - if is_error0 and IS_ABOVE_OPENSSL10: - return EMPTY_RESULT - raise return s, self.get_environ(s) def get_environ(self, sock): diff --git a/lib/cheroot/ssl/pyopenssl.py b/lib/cheroot/ssl/pyopenssl.py index 548200f7..8b01b348 100644 --- a/lib/cheroot/ssl/pyopenssl.py +++ b/lib/cheroot/ssl/pyopenssl.py @@ -150,7 +150,7 @@ class SSLFileobjectMixin: return self._safe_call( False, super(SSLFileobjectMixin, self).sendall, - *args, **kwargs + *args, **kwargs, ) def send(self, *args, **kwargs): @@ -158,7 +158,7 @@ class SSLFileobjectMixin: return self._safe_call( False, super(SSLFileobjectMixin, self).send, - *args, **kwargs + *args, **kwargs, ) @@ -196,6 +196,7 @@ class SSLConnectionProxyMeta: def lock_decorator(method): """Create a proxy method for a new class.""" + def proxy_wrapper(self, *args): self._lock.acquire() try: @@ -212,6 +213,7 @@ class SSLConnectionProxyMeta: def make_property(property_): """Create a proxy method for a new class.""" + def proxy_prop_wrapper(self): return getattr(self._ssl_conn, property_) proxy_prop_wrapper.__name__ = property_ diff --git a/lib/cheroot/test/conftest.py b/lib/cheroot/test/conftest.py index 205753b1..2beff2bf 100644 --- a/lib/cheroot/test/conftest.py +++ b/lib/cheroot/test/conftest.py @@ -12,7 +12,10 @@ import pytest from .._compat import IS_MACOS, IS_WINDOWS # noqa: WPS436 from ..server import Gateway, HTTPServer from ..testing import ( # noqa: F401 # pylint: disable=unused-import - native_server, wsgi_server, + native_server, + thread_and_wsgi_server, + thread_and_native_server, + wsgi_server, ) from ..testing import get_server_client @@ -31,6 +34,28 @@ def http_request_timeout(): return computed_timeout +@pytest.fixture +# pylint: disable=redefined-outer-name +def wsgi_server_thread(thread_and_wsgi_server): # noqa: F811 + """Set up and tear down a Cheroot WSGI server instance. + + This exposes the server thread. + """ + server_thread, _srv = thread_and_wsgi_server + return server_thread + + +@pytest.fixture +# pylint: disable=redefined-outer-name +def native_server_thread(thread_and_native_server): # noqa: F811 + """Set up and tear down a Cheroot HTTP server instance. + + This exposes the server thread. + """ + server_thread, _srv = thread_and_native_server + return server_thread + + @pytest.fixture # pylint: disable=redefined-outer-name def wsgi_server_client(wsgi_server): # noqa: F811 diff --git a/lib/cheroot/test/test_conn.py b/lib/cheroot/test/test_conn.py index f2d2cdb2..21dff855 100644 --- a/lib/cheroot/test/test_conn.py +++ b/lib/cheroot/test/test_conn.py @@ -1,7 +1,9 @@ """Tests for TCP connection handling, including proper and timely close.""" import errno +from re import match as _matches_pattern import socket +import sys import time import logging import traceback as traceback_ @@ -17,6 +19,7 @@ from cheroot._compat import IS_CI, IS_MACOS, IS_PYPY, IS_WINDOWS import cheroot.server +IS_PY36 = sys.version_info[:2] == (3, 6) IS_SLOW_ENV = IS_MACOS or IS_WINDOWS @@ -53,7 +56,8 @@ class Controller(helper.Controller): "'POST' != request.method %r" % req.environ['REQUEST_METHOD'], ) - return "thanks for '%s'" % req.environ['wsgi.input'].read() + input_contents = req.environ['wsgi.input'].read().decode('utf-8') + return f"thanks for '{input_contents !s}'" def custom_204(req, resp): """Render response with status 204.""" @@ -605,18 +609,18 @@ def test_keepalive_conn_management(test_client): pytest.param(RuntimeError, 666, True, id='RuntimeError(666)'), pytest.param(socket.error, -1, True, id='socket.error(-1)'), ) + ( - pytest.param( - ConnectionResetError, errno.ECONNRESET, False, - id='ConnectionResetError(ECONNRESET)', - ), - pytest.param( - BrokenPipeError, errno.EPIPE, False, - id='BrokenPipeError(EPIPE)', - ), - pytest.param( - BrokenPipeError, errno.ESHUTDOWN, False, - id='BrokenPipeError(ESHUTDOWN)', - ), + pytest.param( + ConnectionResetError, errno.ECONNRESET, False, + id='ConnectionResetError(ECONNRESET)', + ), + pytest.param( + BrokenPipeError, errno.EPIPE, False, + id='BrokenPipeError(EPIPE)', + ), + pytest.param( + BrokenPipeError, errno.ESHUTDOWN, False, + id='BrokenPipeError(ESHUTDOWN)', + ), ), ) def test_broken_connection_during_tcp_fin( @@ -699,6 +703,275 @@ def test_broken_connection_during_tcp_fin( assert _close_kernel_socket.exception_leaked is exception_leaks +def test_broken_connection_during_http_communication_fallback( # noqa: WPS118 + monkeypatch, + test_client, + testing_server, + wsgi_server_thread, +): + """Test that unhandled internal error cascades into shutdown.""" + def _raise_connection_reset(*_args, **_kwargs): + raise ConnectionResetError(666) + + def _read_request_line(self): + monkeypatch.setattr(self.conn.rfile, 'close', _raise_connection_reset) + monkeypatch.setattr(self.conn.wfile, 'write', _raise_connection_reset) + _raise_connection_reset() + + monkeypatch.setattr( + test_client.server_instance.ConnectionClass.RequestHandlerClass, + 'read_request_line', + _read_request_line, + ) + + test_client.get_connection().send(b'GET / HTTP/1.1') + wsgi_server_thread.join() # no extra logs upon server termination + + actual_log_entries = testing_server.error_log.calls[:] + testing_server.error_log.calls.clear() # prevent post-test assertions + + expected_log_entries = ( + (logging.WARNING, r'^socket\.error 666$'), + ( + logging.INFO, + '^Got a connection error while handling a connection ' + r'from .*:\d{1,5} \(666\)', + ), + ( + logging.CRITICAL, + r'A fatal exception happened\. Setting the server interrupt flag ' + r'to ConnectionResetError\(666,?\) and giving up\.\n\nPlease, ' + 'report this on the Cheroot tracker at ' + r', ' + 'providing a full reproducer with as much context and details ' + r'as possible\.$', + ), + ) + + assert len(actual_log_entries) == len(expected_log_entries) + + for ( # noqa: WPS352 + (expected_log_level, expected_msg_regex), + (actual_msg, actual_log_level, _tb), + ) in zip(expected_log_entries, actual_log_entries): + assert expected_log_level == actual_log_level + assert _matches_pattern(expected_msg_regex, actual_msg) is not None, ( + f'{actual_msg !r} does not match {expected_msg_regex !r}' + ) + + +def test_kb_int_from_http_handler( + test_client, + testing_server, + wsgi_server_thread, +): + """Test that a keyboard interrupt from HTTP handler causes shutdown.""" + def _trigger_kb_intr(_req, _resp): + raise KeyboardInterrupt('simulated test handler keyboard interrupt') + testing_server.wsgi_app.handlers['/kb_intr'] = _trigger_kb_intr + + http_conn = test_client.get_connection() + http_conn.putrequest('GET', '/kb_intr', skip_host=True) + http_conn.putheader('Host', http_conn.host) + http_conn.endheaders() + wsgi_server_thread.join() # no extra logs upon server termination + + actual_log_entries = testing_server.error_log.calls[:] + testing_server.error_log.calls.clear() # prevent post-test assertions + + expected_log_entries = ( + ( + logging.DEBUG, + '^Got a server shutdown request while handling a connection ' + r'from .*:\d{1,5} \(simulated test handler keyboard interrupt\)$', + ), + ( + logging.DEBUG, + '^Setting the server interrupt flag to KeyboardInterrupt' + r"\('simulated test handler keyboard interrupt',?\)$", + ), + ( + logging.INFO, + '^Keyboard Interrupt: shutting down$', + ), + ) + + assert len(actual_log_entries) == len(expected_log_entries) + + for ( # noqa: WPS352 + (expected_log_level, expected_msg_regex), + (actual_msg, actual_log_level, _tb), + ) in zip(expected_log_entries, actual_log_entries): + assert expected_log_level == actual_log_level + assert _matches_pattern(expected_msg_regex, actual_msg) is not None, ( + f'{actual_msg !r} does not match {expected_msg_regex !r}' + ) + + +@pytest.mark.xfail( + IS_CI and IS_PYPY and IS_PY36 and not IS_SLOW_ENV, + reason='Fails under PyPy 3.6 under Ubuntu 20.04 in CI for unknown reason', + # NOTE: Actually covers any Linux + strict=False, +) +def test_unhandled_exception_in_request_handler( + mocker, + monkeypatch, + test_client, + testing_server, + wsgi_server_thread, +): + """Ensure worker threads are resilient to in-handler exceptions.""" + + class SillyMistake(BaseException): # noqa: WPS418, WPS431 + """A simulated crash within an HTTP handler.""" + + def _trigger_scary_exc(_req, _resp): + raise SillyMistake('simulated unhandled exception ๐Ÿ’ฃ in test handler') + + testing_server.wsgi_app.handlers['/scary_exc'] = _trigger_scary_exc + + server_connection_close_spy = mocker.spy( + test_client.server_instance.ConnectionClass, + 'close', + ) + + http_conn = test_client.get_connection() + http_conn.putrequest('GET', '/scary_exc', skip_host=True) + http_conn.putheader('Host', http_conn.host) + http_conn.endheaders() + + # NOTE: This spy ensure the log entry gets recorded before we're testing + # NOTE: them and before server shutdown, preserving their order and making + # NOTE: the log entry presence non-flaky. + while not server_connection_close_spy.called: # noqa: WPS328 + pass + + assert len(testing_server.requests._threads) == 10 + while testing_server.requests.idle < 10: # noqa: WPS328 + pass + assert len(testing_server.requests._threads) == 10 + testing_server.interrupt = SystemExit('test requesting shutdown') + assert not testing_server.requests._threads + wsgi_server_thread.join() # no extra logs upon server termination + + actual_log_entries = testing_server.error_log.calls[:] + testing_server.error_log.calls.clear() # prevent post-test assertions + + expected_log_entries = ( + ( + logging.ERROR, + '^Unhandled error while processing an incoming connection ' + 'SillyMistake' + r"\('simulated unhandled exception ๐Ÿ’ฃ in test handler',?\)$", + ), + ( + logging.INFO, + '^SystemExit raised: shutting down$', + ), + ) + + assert len(actual_log_entries) == len(expected_log_entries) + + for ( # noqa: WPS352 + (expected_log_level, expected_msg_regex), + (actual_msg, actual_log_level, _tb), + ) in zip(expected_log_entries, actual_log_entries): + assert expected_log_level == actual_log_level + assert _matches_pattern(expected_msg_regex, actual_msg) is not None, ( + f'{actual_msg !r} does not match {expected_msg_regex !r}' + ) + + +@pytest.mark.xfail( + IS_CI and IS_PYPY and IS_PY36 and not IS_SLOW_ENV, + reason='Fails under PyPy 3.6 under Ubuntu 20.04 in CI for unknown reason', + # NOTE: Actually covers any Linux + strict=False, +) +def test_remains_alive_post_unhandled_exception( + mocker, + monkeypatch, + test_client, + testing_server, + wsgi_server_thread, +): + """Ensure worker threads are resilient to unhandled exceptions.""" + + class ScaryCrash(BaseException): # noqa: WPS418, WPS431 + """A simulated crash during HTTP parsing.""" + + _orig_read_request_line = ( + test_client.server_instance. + ConnectionClass.RequestHandlerClass. + read_request_line + ) + + def _read_request_line(self): + _orig_read_request_line(self) + raise ScaryCrash(666) + + monkeypatch.setattr( + test_client.server_instance.ConnectionClass.RequestHandlerClass, + 'read_request_line', + _read_request_line, + ) + + server_connection_close_spy = mocker.spy( + test_client.server_instance.ConnectionClass, + 'close', + ) + + # NOTE: The initial worker thread count is 10. + assert len(testing_server.requests._threads) == 10 + + test_client.get_connection().send(b'GET / HTTP/1.1') + + # NOTE: This spy ensure the log entry gets recorded before we're testing + # NOTE: them and before server shutdown, preserving their order and making + # NOTE: the log entry presence non-flaky. + while not server_connection_close_spy.called: # noqa: WPS328 + pass + + # NOTE: This checks for whether there's any crashed threads + while testing_server.requests.idle < 10: # noqa: WPS328 + pass + assert len(testing_server.requests._threads) == 10 + assert all( + worker_thread.is_alive() + for worker_thread in testing_server.requests._threads + ) + testing_server.interrupt = SystemExit('test requesting shutdown') + assert not testing_server.requests._threads + wsgi_server_thread.join() # no extra logs upon server termination + + actual_log_entries = testing_server.error_log.calls[:] + testing_server.error_log.calls.clear() # prevent post-test assertions + + expected_log_entries = ( + ( + logging.ERROR, + '^Unhandled error while processing an incoming connection ' + r'ScaryCrash\(666,?\)$', + ), + ( + logging.INFO, + '^SystemExit raised: shutting down$', + ), + ) + + assert len(actual_log_entries) == len(expected_log_entries) + + for ( # noqa: WPS352 + (expected_log_level, expected_msg_regex), + (actual_msg, actual_log_level, _tb), + ) in zip(expected_log_entries, actual_log_entries): + assert expected_log_level == actual_log_level + assert _matches_pattern(expected_msg_regex, actual_msg) is not None, ( + f'{actual_msg !r} does not match {expected_msg_regex !r}' + ) + + @pytest.mark.parametrize( 'timeout_before_headers', ( @@ -917,7 +1190,7 @@ def test_100_Continue(test_client): status_line, _actual_headers, actual_resp_body = webtest.shb(response) actual_status = int(status_line[:3]) assert actual_status == 200 - expected_resp_body = ("thanks for '%s'" % body).encode() + expected_resp_body = f"thanks for '{body.decode() !s}'".encode() assert actual_resp_body == expected_resp_body conn.close() @@ -987,7 +1260,7 @@ def test_readall_or_close(test_client, max_request_body_size): status_line, actual_headers, actual_resp_body = webtest.shb(response) actual_status = int(status_line[:3]) assert actual_status == 200 - expected_resp_body = ("thanks for '%s'" % body).encode() + expected_resp_body = f"thanks for '{body.decode() !s}'".encode() assert actual_resp_body == expected_resp_body conn.close() diff --git a/lib/cheroot/test/test_core.py b/lib/cheroot/test/test_core.py index 7732b6f7..d3364788 100644 --- a/lib/cheroot/test/test_core.py +++ b/lib/cheroot/test/test_core.py @@ -134,7 +134,7 @@ def test_query_string_request(test_client): '/hello', # plain '/query_string?test=True', # query '/{0}?{1}={2}'.format( # quoted unicode - *map(urllib.parse.quote, ('ะฎั…ั…ัƒัƒัƒ', 'ั—', 'ะนะพ')) + *map(urllib.parse.quote, ('ะฎั…ั…ัƒัƒัƒ', 'ั—', 'ะนะพ')), ), ), ) diff --git a/lib/cheroot/testing.py b/lib/cheroot/testing.py index 3e404e59..5457a4b1 100644 --- a/lib/cheroot/testing.py +++ b/lib/cheroot/testing.py @@ -31,7 +31,7 @@ config = { @contextmanager -def cheroot_server(server_factory): +def cheroot_server(server_factory): # noqa: WPS210 """Set up and tear down a Cheroot server instance.""" conf = config[server_factory].copy() bind_port = conf.pop('bind_addr')[-1] @@ -41,7 +41,7 @@ def cheroot_server(server_factory): actual_bind_addr = (interface, bind_port) httpserver = server_factory( # create it bind_addr=actual_bind_addr, - **conf + **conf, ) except OSError: pass @@ -50,27 +50,52 @@ def cheroot_server(server_factory): httpserver.shutdown_timeout = 0 # Speed-up tests teardown - threading.Thread(target=httpserver.safe_start).start() # spawn it + # FIXME: Expose this thread through a fixture so that it + # FIXME: could be awaited in tests. + server_thread = threading.Thread(target=httpserver.safe_start) + server_thread.start() # spawn it while not httpserver.ready: # wait until fully initialized and bound time.sleep(0.1) - yield httpserver - - httpserver.stop() # destroy it + try: + yield server_thread, httpserver + finally: + httpserver.stop() # destroy it + server_thread.join() # wait for the thread to be turn down @pytest.fixture -def wsgi_server(): +def thread_and_wsgi_server(): + """Set up and tear down a Cheroot WSGI server instance. + + This emits a tuple of a thread and a server instance. + """ + with cheroot_server(cheroot.wsgi.Server) as (server_thread, srv): + yield server_thread, srv + + +@pytest.fixture +def thread_and_native_server(): + """Set up and tear down a Cheroot HTTP server instance. + + This emits a tuple of a thread and a server instance. + """ + with cheroot_server(cheroot.server.HTTPServer) as (server_thread, srv): + yield server_thread, srv + + +@pytest.fixture +def wsgi_server(thread_and_wsgi_server): # noqa: WPS442 """Set up and tear down a Cheroot WSGI server instance.""" - with cheroot_server(cheroot.wsgi.Server) as srv: - yield srv + _server_thread, srv = thread_and_wsgi_server + return srv @pytest.fixture -def native_server(): +def native_server(thread_and_native_server): # noqa: WPS442 """Set up and tear down a Cheroot HTTP server instance.""" - with cheroot_server(cheroot.server.HTTPServer) as srv: - yield srv + _server_thread, srv = thread_and_native_server + return srv class _TestClient: diff --git a/lib/cheroot/workers/threadpool.py b/lib/cheroot/workers/threadpool.py index 3437d9bd..cd28450a 100644 --- a/lib/cheroot/workers/threadpool.py +++ b/lib/cheroot/workers/threadpool.py @@ -6,6 +6,7 @@ """ import collections +import logging import threading import time import socket @@ -30,7 +31,7 @@ class TrueyZero: trueyzero = TrueyZero() -_SHUTDOWNREQUEST = None +_SHUTDOWNREQUEST = object() class WorkerThread(threading.Thread): @@ -99,39 +100,127 @@ class WorkerThread(threading.Thread): threading.Thread.__init__(self) def run(self): - """Process incoming HTTP connections. + """Set up incoming HTTP connection processing loop. - Retrieves incoming connections from thread pool. + This is the thread's entry-point. It performs lop-layer + exception handling and interrupt processing. + :exc:`KeyboardInterrupt` and :exc:`SystemExit` bubbling up + from the inner-layer code constitute a global server interrupt + request. When they happen, the worker thread exits. + + :raises BaseException: when an unexpected non-interrupt + exception leaks from the inner layers + + # noqa: DAR401 KeyboardInterrupt SystemExit """ self.server.stats['Worker Threads'][self.name] = self.stats + self.ready = True try: - self.ready = True - while True: - conn = self.server.requests.get() - if conn is _SHUTDOWNREQUEST: - return + self._process_connections_until_interrupted() + except (KeyboardInterrupt, SystemExit) as interrupt_exc: + interrupt_cause = interrupt_exc.__cause__ or interrupt_exc + self.server.error_log( + f'Setting the server interrupt flag to {interrupt_cause !r}', + level=logging.DEBUG, + ) + self.server.interrupt = interrupt_cause + except BaseException as underlying_exc: # noqa: WPS424 + # NOTE: This is the last resort logging with the last dying breath + # NOTE: of the worker. It is only reachable when exceptions happen + # NOTE: in the `finally` branch of the internal try/except block. + self.server.error_log( + 'A fatal exception happened. Setting the server interrupt flag' + f' to {underlying_exc !r} and giving up.' + '\N{NEW LINE}\N{NEW LINE}' + 'Please, report this on the Cheroot tracker at ' + ', ' + 'providing a full reproducer with as much context and details as possible.', + level=logging.CRITICAL, + traceback=True, + ) + self.server.interrupt = underlying_exc + raise + finally: + self.ready = False - self.conn = conn - is_stats_enabled = self.server.stats['Enabled'] + def _process_connections_until_interrupted(self): + """Process incoming HTTP connections in an infinite loop. + + Retrieves incoming connections from thread pool, processing + them one by one. + + :raises SystemExit: on the internal requests to stop the + server instance + """ + while True: + conn = self.server.requests.get() + if conn is _SHUTDOWNREQUEST: + return + + self.conn = conn + is_stats_enabled = self.server.stats['Enabled'] + if is_stats_enabled: + self.start_time = time.time() + keep_conn_open = False + try: + keep_conn_open = conn.communicate() + except ConnectionError as connection_error: + keep_conn_open = False # Drop the connection cleanly + self.server.error_log( + 'Got a connection error while handling a ' + f'connection from {conn.remote_addr !s}:' + f'{conn.remote_port !s} ({connection_error !s})', + level=logging.INFO, + ) + continue + except (KeyboardInterrupt, SystemExit) as shutdown_request: + # Shutdown request + keep_conn_open = False # Drop the connection cleanly + self.server.error_log( + 'Got a server shutdown request while handling a ' + f'connection from {conn.remote_addr !s}:' + f'{conn.remote_port !s} ({shutdown_request !s})', + level=logging.DEBUG, + ) + raise SystemExit( + str(shutdown_request), + ) from shutdown_request + except BaseException as unhandled_error: # noqa: WPS424 + # NOTE: Only a shutdown request should bubble up to the + # NOTE: external cleanup code. Otherwise, this thread dies. + # NOTE: If this were to happen, the threadpool would still + # NOTE: list a dead thread without knowing its state. And + # NOTE: the calling code would fail to schedule processing + # NOTE: of new requests. + self.server.error_log( + 'Unhandled error while processing an incoming ' + f'connection {unhandled_error !r}', + level=logging.ERROR, + traceback=True, + ) + continue # Prevent the thread from dying + finally: + # NOTE: Any exceptions coming from within `finally` may + # NOTE: kill the thread, causing the threadpool to only + # NOTE: contain references to dead threads rendering the + # NOTE: server defunct, effectively meaning a DoS. + # NOTE: Ideally, things called here should process + # NOTE: everything recoverable internally. Any unhandled + # NOTE: errors will bubble up into the outer try/except + # NOTE: block. They will be treated as fatal and turned + # NOTE: into server shutdown requests and then reraised + # NOTE: unconditionally. + if keep_conn_open: + self.server.put_conn(conn) + else: + conn.close() if is_stats_enabled: - self.start_time = time.time() - keep_conn_open = False - try: - keep_conn_open = conn.communicate() - finally: - if keep_conn_open: - self.server.put_conn(conn) - else: - conn.close() - if is_stats_enabled: - self.requests_seen += self.conn.requests_seen - self.bytes_read += self.conn.rfile.bytes_read - self.bytes_written += self.conn.wfile.bytes_written - self.work_time += time.time() - self.start_time - self.start_time = None - self.conn = None - except (KeyboardInterrupt, SystemExit) as ex: - self.server.interrupt = ex + self.requests_seen += conn.requests_seen + self.bytes_read += conn.rfile.bytes_read + self.bytes_written += conn.wfile.bytes_written + self.work_time += time.time() - self.start_time + self.start_time = None + self.conn = None class ThreadPool: diff --git a/requirements.txt b/requirements.txt index 7b261f3d..ea940f73 100644 --- a/requirements.txt +++ b/requirements.txt @@ -4,7 +4,7 @@ backports.zoneinfo==0.2.1;python_version<"3.9" beautifulsoup4==4.12.3 bleach==6.1.0 certifi==2024.2.2 -cheroot==10.0.0 +cheroot==10.0.1 cherrypy==18.9.0 cloudinary==1.40.0 distro==1.9.0