From 342ecf60ee8078433ab2acaea8e2c4bac662b9de Mon Sep 17 00:00:00 2001 From: julianz- <6255571+julianz-@users.noreply.github.com> Date: Sun, 28 Sep 2025 22:26:54 -0700 Subject: [PATCH] Fix race condition and improve robustness during socket I/O Fixes to make socket I/O more resilient during connection teardown. 1. BufferedWriter's write(): Added error handling to ignore common socket errors (e.g., ECONNRESET, EPIPE, ENOTCONN, EBADF) that occur when the underlying connection has been unexpectedly closed by the client or OS. This prevents a crash when attempting to write to a defunct socket. 2. BufferedWriters's close(): Made idempotent, allowing safe repeated calls without raising exceptions. 3. Needed to add explicit handling of WINDOWS environments as these are seen to throw Windows specific WSAENOTSOCK errors. Includes new unit tests to cover the idempotency and graceful handling of already closed underlying buffers. --- .flake8 | 2 +- cheroot/errors.py | 12 ++- cheroot/errors.pyi | 5 +- cheroot/makefile.py | 36 +++++++ cheroot/test/test_makefile.py | 121 +++++++++++++++++++++- docs/changelog-fragments.d/779.bugfix.rst | 4 + docs/spelling_wordlist.txt | 1 + 7 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 docs/changelog-fragments.d/779.bugfix.rst diff --git a/.flake8 b/.flake8 index aca9b2fffe..c0f283e474 100644 --- a/.flake8 +++ b/.flake8 @@ -133,7 +133,7 @@ per-file-ignores = cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS325, WPS354, WPS420, WPS422, WPS430, WPS457 cheroot/test/helper.py: DAR101, DAR201, DAR401, I001, I003, I004, N802, WPS110, WPS111, WPS121, WPS201, WPS220, WPS231, WPS301, WPS414, WPS421, WPS422, WPS505 cheroot/test/test_cli.py: DAR101, DAR201, I001, I005, N802, S101, S108, WPS110, WPS421, WPS431, WPS473 - cheroot/test/test_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122 + cheroot/test/test_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122, WPS202 cheroot/test/test_wsgi.py: DAR101, DAR301, I001, I004, S101, WPS110, WPS111, WPS117, WPS118, WPS121, WPS210, WPS421, WPS430, WPS432, WPS441, WPS509 cheroot/test/test_core.py: C815, DAR101, DAR201, DAR401, I003, I004, N805, N806, S101, WPS110, WPS111, WPS114, WPS121, WPS202, WPS204, WPS226, WPS229, WPS324, WPS421, WPS422, WPS432, WPS602 cheroot/test/test_dispatch.py: DAR101, DAR201, S101, WPS111, WPS121, WPS422, WPS430 diff --git a/cheroot/errors.py b/cheroot/errors.py index a1103595c2..5e6690ccc2 100644 --- a/cheroot/errors.py +++ b/cheroot/errors.py @@ -3,6 +3,8 @@ import errno import sys +from . import _compat + class MaxSizeExceeded(Exception): """Exception raised when a client sends more data then allowed under limit. @@ -66,19 +68,23 @@ def plat_specific_errors(*errnames): acceptable_sock_shutdown_error_codes = { + errno.EBADF, errno.ENOTCONN, errno.EPIPE, errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3 errno.ECONNRESET, # corresponds to ConnectionResetError in Python 3 + *((errno.WSAENOTSOCK,) if _compat.IS_WINDOWS else ()), } """Errors that may happen during the connection close sequence. +* EBADF - raised when operating on a closed socket * ENOTCONN — client is no longer connected * EPIPE — write on a pipe while the other end has been closed * ESHUTDOWN — write on a socket which has been shutdown for writing * ECONNRESET — connection is reset by the peer, we received a TCP RST packet Refs: + * https://github.com/cherrypy/cheroot/issues/341#issuecomment-735884889 * https://bugs.python.org/issue30319 * https://bugs.python.org/issue30329 @@ -87,4 +93,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, # Covers EPIPE and ESHUTDOWN + ConnectionResetError, # Covers ECONNRESET +) diff --git a/cheroot/errors.pyi b/cheroot/errors.pyi index 186695682f..4c8d490f6f 100644 --- a/cheroot/errors.pyi +++ b/cheroot/errors.pyi @@ -10,4 +10,7 @@ socket_error_eintr: List[int] socket_errors_to_ignore: List[int] socket_errors_nonblocking: List[int] acceptable_sock_shutdown_error_codes: Set[int] -acceptable_sock_shutdown_exceptions: Tuple[Type[Exception], ...] +acceptable_sock_shutdown_exceptions: Tuple[ + Type[BrokenPipeError], + Type[ConnectionResetError], +] diff --git a/cheroot/makefile.py b/cheroot/makefile.py index f5780a1ede..2dd0b8c379 100644 --- a/cheroot/makefile.py +++ b/cheroot/makefile.py @@ -4,6 +4,8 @@ import _pyio as io import socket +from . import errors as _errors + # Write only 16K at a time to sockets SOCK_WRITE_BLOCKSIZE = 16384 @@ -32,8 +34,42 @@ def _flush_unlocked(self): n = self.raw.write(bytes(self._write_buf)) except io.BlockingIOError as e: n = e.characters_written + + if n == 0: + # If nothing was written we need to break + # to avoid infinte loops + 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 :exc:`ConnectionError` or :exc:`OSError` with + :data:`~errno.EBADF` or :data:`~errno.ENOTCONN` is caught + and ignored, as these indicate a normal, expected connection teardown. + Other exceptions are re-raised. + """ + # pylint incorrectly flags inherited self.closed property as constant + if self.closed: # pylint: disable=using-constant-test + return + + try: + super().close() + except ConnectionError: + return + except OSError as err: + # Handle EBADF and other acceptable socket shutdown errors + if err.errno in _errors.acceptable_sock_shutdown_error_codes: + return + raise + class StreamReader(io.BufferedReader): """Socket stream reader.""" diff --git a/cheroot/test/test_makefile.py b/cheroot/test/test_makefile.py index d65d4ea268..d4e80415a8 100644 --- a/cheroot/test/test_makefile.py +++ b/cheroot/test/test_makefile.py @@ -1,10 +1,16 @@ """Tests for :py:mod:`cheroot.makefile`.""" +import errno +import io +import math + +import pytest + from cheroot import makefile class MockSocket: - """A mock socket.""" + """A mock socket for emulating buffered I/O.""" def __init__(self): """Initialize :py:class:`MockSocket`.""" @@ -51,3 +57,116 @@ def test_bytes_written(): wfile = makefile.MakeFile(sock, 'w') wfile.write(b'bar') assert wfile.bytes_written == 3 + + +def test_close_is_idempotent(): + """Test that double ``close()`` does not error out.""" + raw_buffer = io.BytesIO() + buffered_writer = makefile.BufferedWriter(raw_buffer) + + # Should not raise any exceptions + buffered_writer.close() + assert buffered_writer.closed + + buffered_writer.close() # Second call should be safe + assert buffered_writer.closed + + +def test_close_handles_already_closed_buffer(): + """Test that ``close()`` handles already closed underlying buffer.""" + raw_buffer = io.BytesIO() + buffered_writer = makefile.BufferedWriter(raw_buffer) + + # Close the underlying buffer first + raw_buffer.close() + + # This should not raise an exception + assert raw_buffer.closed + assert buffered_writer.closed + + +def test_flush_unlocked_handles_blocking_io_error(mock_buffer_writer, mocker): + """ + Test that a BlockingIOError is handled correctly. + + We extracting characters_written, + and execution continues without raising the error. + """ + # 1. Create a mock object to replace the real 'write' method + mock_write_method = mocker.Mock() + + # 2. Set the side effect on the mock object + err = io.BlockingIOError(errno.EAGAIN, 'Resource temporarily unavailable') + err.characters_written = 5 + mock_write_method.side_effect = err + + # 3. Use mocker.patch.object to replace the 'write' method + # with mock_write_method + mocker.patch.object(mock_buffer_writer.raw, 'write', new=mock_write_method) + + # Check the initial state of the buffer + initial_len = len(mock_buffer_writer._write_buf) + + # 4. Execute the code + try: + mock_buffer_writer._flush_unlocked() + except Exception as exc: + pytest.fail(f'Unexpected exception raised: {type(exc).__name__}') + + # 5. Verify the side-effect (buffer should be empty) + assert len(mock_buffer_writer._write_buf) == 0 + + # 6 Check mock calls (Logic/Mechanism) + # The number of calls should be + # initial_len / bytes_written_per_call + expected_calls = math.ceil(initial_len / 5) + assert mock_write_method.call_count == expected_calls + + +class MockRawSocket: + """ + A mock raw socket for emulating low level unbuffered I/O. + + We use this mock with ``io.BufferedWriter``, which accesses it via + the ``.raw`` attribute. + """ + + def __init__(self, *args, **kwargs): + """Initialize :py:class:`MockRawSocket`.""" + # 1. Call the parent's init to set up self.messages + super().__init__(*args, **kwargs) + + # 2. Rquired by the io.BufferedWriter base class + self._is_closed = False + + def write(self, message): + """Emulate ``io.RawIOBase write``.""" + # Use the underlying send method implemented in MockSocket + return self.send(message) + + def writable(self): + """Indicate that the raw stream supports writing.""" + return True + + def send(self, message): + """Emulate a send.""" + return len(message) + + def close(self): + """Emulate close.""" + self._is_closed = True + + @property + def closed(self): + """Emulate the required ``closed`` property.""" + return self._is_closed + + +@pytest.fixture +def mock_buffer_writer(): + """Fixture to create a BufferedWriter instance with a mock raw socket.""" + # Create a BufferedWriter instance with a buffer that has content + # to ensure _flush_unlocked attempts to write. + writer = makefile.BufferedWriter(MockRawSocket()) + writer._write_buf = bytearray(b'data to flush') + return writer diff --git a/docs/changelog-fragments.d/779.bugfix.rst b/docs/changelog-fragments.d/779.bugfix.rst new file mode 100644 index 0000000000..615d08368b --- /dev/null +++ b/docs/changelog-fragments.d/779.bugfix.rst @@ -0,0 +1,4 @@ +Socket I/O is now resilient to race conditions happening during connection teardown +due to sockets dying independently or being closed externally. + +-- by :user:`julianz-` diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 1e1022c82f..ce4c141c04 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -65,6 +65,7 @@ subpackages symlinked syscall systemd +teardown threadpool Tidelift TLS