From 7fb95c23e9544914421abf803803a6191118f815 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 | 16 +++- cheroot/errors.pyi | 7 +- cheroot/makefile.py | 39 ++++++++ cheroot/test/test_makefile.py | 108 +++++++++++++++++++++- docs/changelog-fragments.d/779.bugfix.rst | 4 + docs/spelling_wordlist.txt | 1 + 7 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 docs/changelog-fragments.d/779.bugfix.rst diff --git a/.flake8 b/.flake8 index 6ab1219ead..b5eec11702 100644 --- a/.flake8 +++ b/.flake8 @@ -138,7 +138,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, DAR401, 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..a65e641aa8 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,6 +68,10 @@ def plat_specific_errors(*errnames): acceptable_sock_shutdown_error_codes = { + errno.EBADF, # operating on a closed/invalid file descriptor + *( + (errno.WSAENOTSOCK,) if _compat.IS_WINDOWS else () + ), # Windows equivalent of EBADF errno.ENOTCONN, errno.EPIPE, errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3 @@ -73,12 +79,16 @@ def plat_specific_errors(*errnames): } """Errors that may happen during the connection close sequence. +* EBADF — operating on a closed or invalid file descriptor +* WSAENOTSOCK — Windows equivalent of EBADF; raised when the socket has + already been closed by the OS or another thread * 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 +97,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 dd60f7a4a4..e5fc96a97c 100644 --- a/cheroot/errors.pyi +++ b/cheroot/errors.pyi @@ -1,5 +1,3 @@ -import typing as _t - class MaxSizeExceeded(Exception): ... class NoSSLError(Exception): ... class FatalSSLAlert(Exception): ... @@ -10,4 +8,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[_t.Type[Exception], ...] +acceptable_sock_shutdown_exceptions: tuple[ + type[BrokenPipeError], + type[ConnectionResetError], +] diff --git a/cheroot/makefile.py b/cheroot/makefile.py index f5780a1ede..d3d40682de 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,45 @@ def _flush_unlocked(self): n = self.raw.write(bytes(self._write_buf)) except io.BlockingIOError as e: n = e.characters_written + + if n == 0: + # Non-blocking socket can't write right now — stop flushing. + break + if not n: + # Defensive: write() returned None or other falsy value, + # which shouldn't happen but could cause an infinite loop. + 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..2207ae696c 100644 --- a/cheroot/test/test_makefile.py +++ b/cheroot/test/test_makefile.py @@ -1,10 +1,16 @@ """Tests for :py:mod:`cheroot.makefile`.""" +import _pyio +import errno +import io + +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`.""" @@ -35,6 +41,31 @@ def _decref_socketios(self): # Ref: https://github.com/cherrypy/cheroot/issues/734 +class MockRawIO: + """A mock ``io.RawIOBase`` object for use as the raw layer of a ``BufferedWriter``.""" + + def __init__(self): + """Initialize :py:class:`MockRawIO`.""" + self._is_closed = False + + def write(self, message): + """Emulate ``io.RawIOBase write``.""" + return len(message) + + def writable(self): + """Indicate that the raw stream supports writing.""" + return True + + def close(self): + """Emulate close.""" + self._is_closed = True + + @property + def closed(self): + """Emulate the required ``closed`` property.""" + return self._is_closed + + def test_bytes_read(): """Reader should capture bytes read.""" sock = MockSocket() @@ -51,3 +82,78 @@ 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) + + buffered_writer.close() + assert buffered_writer.closed + + buffered_writer.close() + assert buffered_writer.closed + + +def test_close_when_raw_already_closed(): + """Test that ``close()`` is safe when the raw buffer was closed externally. + + Simulates a race where the OS or another thread closed the raw socket + before ``BufferedWriter.close()`` is called. + """ + raw_buffer = io.BytesIO() + buffered_writer = makefile.BufferedWriter(raw_buffer) + + raw_buffer.close() + assert buffered_writer.closed # property reflects raw state + + buffered_writer.close() + + +@pytest.mark.parametrize( + ('exc', 'reraises'), + ( + (BrokenPipeError(errno.EPIPE, 'Broken pipe'), False), + (OSError(errno.EBADF, 'Bad file descriptor'), False), + (OSError(errno.EIO, 'I/O error'), True), + ), + ids=['broken_pipe', 'ebadf', 'unexpected_oserror'], +) +def test_close_error_handling(exc, reraises, mocker): + """Test that expected socket errors are swallowed and others re-raised.""" + writer = makefile.BufferedWriter(MockRawIO()) + mocker.patch.object(_pyio.BufferedWriter, 'close', side_effect=exc) + if reraises: + with pytest.raises(OSError, match='I/O error'): + writer.close() + else: + writer.close() + + +def _make_blocking_io_error(): + """Create a :exc:`BlockingIOError` with ``characters_written`` set.""" + err = io.BlockingIOError(errno.EAGAIN, 'Resource temporarily unavailable') + err.characters_written = 5 + return err + + +@pytest.mark.parametrize( + ('write_kwargs', 'expected_buf_len'), + ( + ({'return_value': 0}, len(b'data to flush')), + ({'return_value': None}, len(b'data to flush')), + ({'side_effect': _make_blocking_io_error()}, 0), + ), + ids=['zero_return', 'none_return', 'blocking_io_error'], +) +def test_flush_unlocked_write_outcomes(write_kwargs, expected_buf_len, mocker): + """Test that ``_flush_unlocked`` handles various ``write()`` outcomes.""" + data = b'data to flush' + writer = makefile.BufferedWriter(MockRawIO()) + writer._write_buf = bytearray(data) + mocker.patch.object(writer.raw, 'write', **write_kwargs) + + writer._flush_unlocked() + + assert len(writer._write_buf) == expected_buf_len 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 06b204518e..5305595e22 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -67,6 +67,7 @@ subpackages symlinked syscall systemd +teardown threadpool Tidelift TLS