diff --git a/.github/workflows/ci-cd.yml b/.github/workflows/ci-cd.yml index 6cf9b318e2..f8f037be9f 100644 --- a/.github/workflows/ci-cd.yml +++ b/.github/workflows/ci-cd.yml @@ -13,7 +13,7 @@ on: - patchback/backports/** # Patchback always creates PRs - pre-commit-ci-update-config # pre-commit.ci always creates a PR pull_request: - ignore-paths: # changes to the cron workflow are triggered through it + paths-ignore: # changes to the cron workflow are triggered through it - .github/workflows/scheduled-runs.yml workflow_call: # a way to embed the main tests workflow_dispatch: @@ -83,6 +83,7 @@ env: PYTHONIOENCODING PYTHONLEGACYWINDOWSSTDIO PYTHONUTF8 + PYTHONWARNINGS: "ignore::ResourceWarning:sqlite3" TOX_VERSION: tox < 4.12 UPSTREAM_REPOSITORY_ID: >- 16620627 diff --git a/cheroot/connections.pyi b/cheroot/connections.pyi index 528ad76519..794903736b 100644 --- a/cheroot/connections.pyi +++ b/cheroot/connections.pyi @@ -1,5 +1,7 @@ from typing import Any +IS_WINDOWS: bool + def prevent_socket_inheritance(sock) -> None: ... class _ThreadsafeSelector: diff --git a/cheroot/errors.py b/cheroot/errors.py index a1103595c2..7fc5f02497 100644 --- a/cheroot/errors.py +++ b/cheroot/errors.py @@ -4,6 +4,12 @@ import sys +try: + from OpenSSL.SSL import SysCallError as _OpenSSL_SysCallError +except ImportError: + _OpenSSL_SysCallError = None + + class MaxSizeExceeded(Exception): """Exception raised when a client sends more data then allowed under limit. @@ -66,6 +72,7 @@ def plat_specific_errors(*errnames): acceptable_sock_shutdown_error_codes = { + errno.EBADF, errno.ENOTCONN, errno.EPIPE, errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3 @@ -87,4 +94,8 @@ def plat_specific_errors(*errnames): * https://docs.microsoft.com/windows/win32/api/winsock/nf-winsock-shutdown """ -acceptable_sock_shutdown_exceptions = (BrokenPipeError, ConnectionResetError) +acceptable_sock_shutdown_exceptions = ( + BrokenPipeError, + ConnectionResetError, + _OpenSSL_SysCallError, +) diff --git a/cheroot/makefile.py b/cheroot/makefile.py index f5780a1ede..6dd02a30cd 100644 --- a/cheroot/makefile.py +++ b/cheroot/makefile.py @@ -2,8 +2,30 @@ # prefer slower Python-based io module import _pyio as io +import errno import socket +import sys +from OpenSSL.SSL import SysCallError + + +# Define a variable to hold the platform-specific "not a socket" error. +_not_a_socket_err = None + +if sys.platform == 'win32': + # On Windows, try to get the named constant from the socket module. + # If that fails, fall back to the known numeric value. + try: + _not_a_socket_err = socket.WSAENOTSOCK + except AttributeError: + _not_a_socket_err = 10038 +else: + # On other platforms, the relevant error is EBADF (Bad file descriptor), + # which is already in the list of handled errors. + pass + +# Expose the error constant for use in the module's public API if needed. +WSAENOTSOCK = _not_a_socket_err # Write only 16K at a time to sockets SOCK_WRITE_BLOCKSIZE = 16384 @@ -30,10 +52,59 @@ def _flush_unlocked(self): # ssl sockets only except 'bytes', not bytearrays # so perhaps we should conditionally wrap this for perf? n = self.raw.write(bytes(self._write_buf)) - except io.BlockingIOError as e: - n = e.characters_written + except (io.BlockingIOError, OSError, SysCallError) as e: + # Check for a different error attribute depending + # on the exception type + if isinstance(e, io.BlockingIOError): + n = e.characters_written + else: + error_code = ( + e.errno if isinstance(e, OSError) else e.args[0] + ) + if error_code in { + errno.EBADF, + errno.ENOTCONN, + errno.EPIPE, + WSAENOTSOCK, # Windows-specific error + }: + # The socket is gone, so just ignore this error. + return + raise + else: + # The 'try' block completed without an exception + if n is None: + # This could happen with non-blocking write + # when nothing was written + break + del self._write_buf[:n] + def close(self): + """ + Close the stream and its underlying file object. + + This method is designed to be idempotent (it can be called multiple + times without side effects). It gracefully handles a race condition + where the underlying socket may have already been closed by the remote + client or another thread. + + A SysCallError or OSError with errno.EBADF or errno.ENOTCONN is caught + and ignored, as these indicate a normal, expected connection teardown. + Other exceptions are re-raised. + """ + if self.closed: # pylint: disable=W0125 + return + + try: + super().close() + except (OSError, SysCallError) as e: + error_code = e.errno if isinstance(e, OSError) else e.args[0] + if error_code in {errno.EBADF, errno.ENOTCONN}: + # The socket is already closed, which is expected during + # a race condition. + return + raise + class StreamReader(io.BufferedReader): """Socket stream reader.""" diff --git a/cheroot/makefile.pyi b/cheroot/makefile.pyi index 3f5ea2756b..698a16451b 100644 --- a/cheroot/makefile.pyi +++ b/cheroot/makefile.pyi @@ -1,7 +1,14 @@ import io +import sys +from typing import Optional + +WSAENOTSOCK: Optional[int] SOCK_WRITE_BLOCKSIZE: int +if sys.platform == 'win32': + WIN_SOCKET_NOT_OPEN: Optional[int] + class BufferedWriter(io.BufferedWriter): def write(self, b): ... diff --git a/cheroot/server.py b/cheroot/server.py index c836d07c51..be91f2f90f 100644 --- a/cheroot/server.py +++ b/cheroot/server.py @@ -67,6 +67,7 @@ import contextlib import email.utils +import errno import io import logging import os @@ -81,6 +82,8 @@ import urllib.parse from functools import lru_cache +from OpenSSL.SSL import SysCallError + from . import __version__, connections, errors from ._compat import IS_PPC, bton from .makefile import MakeFile, StreamWriter @@ -1186,12 +1189,25 @@ def ensure_headers_sent(self): def write(self, chunk): """Write unbuffered data to the client.""" - if self.chunked_write and chunk: - chunk_size_hex = hex(len(chunk))[2:].encode('ascii') - buf = [chunk_size_hex, CRLF, chunk, CRLF] - self.conn.wfile.write(EMPTY.join(buf)) - else: - self.conn.wfile.write(chunk) + try: + if self.chunked_write and chunk: + chunk_size_hex = hex(len(chunk))[2:].encode('ascii') + buf = [chunk_size_hex, CRLF, chunk, CRLF] + self.conn.wfile.write(EMPTY.join(buf)) + else: + self.conn.wfile.write(chunk) + except (SysCallError, ConnectionError, OSError) as e: + error_code = e.errno if isinstance(e, OSError) else e.args[0] + if error_code in { + errno.ECONNRESET, + errno.EPIPE, + errno.ENOTCONN, + errno.EBADF, + }: + # The socket is gone, so just ignore this error. + return + + raise def send_headers(self): # noqa: C901 # FIXME """Assert, process, and send the HTTP response message-headers. @@ -1285,7 +1301,27 @@ def send_headers(self): # noqa: C901 # FIXME for k, v in self.outheaders: buf.append(k + COLON + SPACE + v + CRLF) buf.append(CRLF) - self.conn.wfile.write(EMPTY.join(buf)) + try: + self.conn.wfile.write(EMPTY.join(buf)) + except (SysCallError, ConnectionError, OSError) as e: + # We explicitly ignore these errors because they indicate the + # client has already closed the connection, which is a normal + # occurrence during a race condition. + + # The .errno attribute is only available on OSError + # The .args[0] attribute is available on SysCallError + # Check for both cases to handle different exception types + error_code = e.errno if isinstance(e, OSError) else e.args[0] + if error_code in { + errno.ECONNRESET, + errno.EPIPE, + errno.ENOTCONN, + errno.EBADF, + }: + self.close_connection = True + self.conn.close() + return + raise class HTTPConnection: @@ -1541,9 +1577,10 @@ def _close_kernel_socket(self): self.socket.shutdown, ) + acceptable_exceptions = errors.acceptable_sock_shutdown_exceptions try: shutdown(socket.SHUT_RDWR) # actually send a TCP FIN - except errors.acceptable_sock_shutdown_exceptions: + except acceptable_exceptions: # pylint: disable=E0712 pass except socket.error as e: if e.errno not in errors.acceptable_sock_shutdown_error_codes: diff --git a/cheroot/server.pyi b/cheroot/server.pyi index 2a3f34eb3b..c5c0f517f6 100644 --- a/cheroot/server.pyi +++ b/cheroot/server.pyi @@ -1,5 +1,18 @@ from typing import Any +__all__ = ( + 'ChunkedRFile', + 'DropUnderscoreHeaderReader', + 'Gateway', + 'HTTPConnection', + 'HTTPRequest', + 'HTTPServer', + 'HeaderReader', + 'KnownLengthRFile', + 'SizeCheckWrapper', + 'get_ssl_adapter_class', +) + class HeaderReader: def __call__(self, rfile, hdict: Any | None = ...): ... diff --git a/cheroot/ssl/builtin.py b/cheroot/ssl/builtin.py index 1cec20255d..04d5fdf34b 100644 --- a/cheroot/ssl/builtin.py +++ b/cheroot/ssl/builtin.py @@ -286,11 +286,19 @@ def wrap(self, sock): 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') - ) + except ( + ssl.SSLError, + OSError, + ) as generic_tls_error: + # When the client speaks plain HTTP into a TLS-only connection, + # Python's builtin ssl raises an SSLError with `http request` + # in its message. Sometimes, due to a race condition, the socket + # is closed by the time we try to handle the error, resulting in an + # OSError: [Errno 9] Bad file descriptor. + peer_speaks_plain_http_over_https = isinstance( + generic_tls_error, + ssl.SSLError, + ) 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: @@ -299,10 +307,6 @@ def wrap(self, sock): 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 return s, self.get_environ(s) diff --git a/cheroot/test/conftest.py b/cheroot/test/conftest.py index 2e967b0572..db61bb0020 100644 --- a/cheroot/test/conftest.py +++ b/cheroot/test/conftest.py @@ -23,7 +23,7 @@ @pytest.fixture def http_request_timeout(): """Return a common HTTP request timeout for tests with queries.""" - computed_timeout = 0.1 + computed_timeout = 0.5 if IS_MACOS: computed_timeout *= 2 diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py index da9a696ff4..ac9af9c2d5 100644 --- a/cheroot/test/test_conn.py +++ b/cheroot/test/test_conn.py @@ -2,6 +2,7 @@ import errno import http.client +import io import logging import socket import time @@ -14,9 +15,11 @@ import pytest from jaraco.text import trim, unwrap +from OpenSSL.SSL import SysCallError import cheroot.server from cheroot._compat import IS_CI, IS_MACOS, IS_PYPY, IS_WINDOWS +from cheroot.makefile import BufferedWriter from cheroot.test import helper, webtest @@ -1637,3 +1640,72 @@ def test_invalid_selected_connection(test_client, monkeypatch): time.sleep(test_client.server_instance.expiration_interval * 2) assert faux_select.os_error_triggered assert faux_get_map.conn_closed + + +class TestBufferedWriter: + """Test cases for BufferedWriter close() method race condition handling.""" + + def test_close_is_idempotent(self): + """Test that close() can be called multiple times safely.""" + raw_buffer = io.BytesIO() + buffered_writer = BufferedWriter(raw_buffer) + + # Should not raise any exceptions + buffered_writer.close() + buffered_writer.close() # Second call should be safe + + assert buffered_writer.closed + + def test_close_handles_already_closed_buffer(self): + """Test that close() handles already closed underlying buffer.""" + raw_buffer = io.BytesIO() + buffered_writer = BufferedWriter(raw_buffer) + + # Close the underlying buffer first + raw_buffer.close() + + # This should not raise an exception + buffered_writer.close() + + def test_close_handles_os_error_ebadf(self, mocker): + """Test that close() handles OSError with EBADF errno gracefully.""" + raw_buffer = io.BytesIO() + buffered_writer = BufferedWriter(raw_buffer) + + # Mock super().close() to raise OSError with EBADF + mock_super = mocker.patch('cheroot.makefile.super') + mock_super.return_value.close.side_effect = OSError( + errno.EBADF, + 'Bad file descriptor', + ) + + # Should handle EBADF gracefully (no exception raised) + buffered_writer.close() + + def test_close_handles_os_error_enotconn(self, mocker): + """Test that close() handles OSError with ENOTCONN errno gracefully.""" + raw_buffer = io.BytesIO() + buffered_writer = BufferedWriter(raw_buffer) + + # Mock super().close() to raise OSError with ENOTCONN + mock_super = mocker.patch('cheroot.makefile.super') + mock_super.return_value.close.side_effect = OSError( + errno.ENOTCONN, + 'Socket is not connected', + ) + + # Should handle ENOTCONN gracefully (no exception raised) + buffered_writer.close() + + def test_close_handles_syscall_error(self, mocker): + """Test that close() handles SysCallError with expected errno codes.""" + raw_buffer = io.BytesIO() + buffered_writer = BufferedWriter(raw_buffer) + + mock_super = mocker.patch('cheroot.makefile.super') + mock_super.return_value.close.side_effect = SysCallError( + errno.EBADF, + ) # args[0] will be errno.EBADF + + # Should handle gracefully + buffered_writer.close() diff --git a/cheroot/workers/threadpool.pyi b/cheroot/workers/threadpool.pyi index 02a09b6c90..856f8a0cef 100644 --- a/cheroot/workers/threadpool.pyi +++ b/cheroot/workers/threadpool.pyi @@ -1,6 +1,8 @@ import threading from typing import Any +__all__ = ('ThreadPool', 'WorkerThread') + class TrueyZero: def __add__(self, other): ... def __radd__(self, other): ... diff --git a/docs/changelog-fragments.d/770.bugfix.rst b/docs/changelog-fragments.d/770.bugfix.rst new file mode 100644 index 0000000000..ed0af013db --- /dev/null +++ b/docs/changelog-fragments.d/770.bugfix.rst @@ -0,0 +1,4 @@ +Fixed socket teardown errors that were being thrown during +connection cleanup. Also resolved multiple stub file issues +including missing __all__ declarations and +incorrect type definitions that were causing stubtest failures. diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index f9020b9ce6..f47497db26 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -13,7 +13,10 @@ chunked dev docstrings downstreams +EBADF +ENOTCONN environ +errno errored Faux fd @@ -57,12 +60,14 @@ signalling Sigstore ssl stdout +stubtest subclasses submodules subpackages symlinked syscall systemd +teardown threadpool Tidelift TLS diff --git a/pytest.ini b/pytest.ini index 6586702144..562ef8b585 100644 --- a/pytest.ini +++ b/pytest.ini @@ -62,6 +62,8 @@ filterwarnings = # Ref: https://github.com/cherrypy/cheroot/issues/734 ignore:Exception ignored in. =3.0.0 commands_pre = {envpython} \ {[python-cli-options]byte-errors} \ @@ -131,7 +132,6 @@ setenv = PYTHONDONTWRITEBYTECODE=x WEBTEST_INTERACTIVE=false - [dists] setenv = PIP_CONSTRAINT = {toxinidir}{/}requirements{/}dist-build-constraints.txt