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