Skip to content

Commit 97cad94

Browse files
committed
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.
1 parent 4a8dc43 commit 97cad94

File tree

8 files changed

+180
-4
lines changed

8 files changed

+180
-4
lines changed

.flake8

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ per-file-ignores =
133133
cheroot/test/conftest.py: DAR101, DAR201, DAR301, I001, I003, I005, WPS100, WPS130, WPS325, WPS354, WPS420, WPS422, WPS430, WPS457
134134
cheroot/test/helper.py: DAR101, DAR201, DAR401, I001, I003, I004, N802, WPS110, WPS111, WPS121, WPS201, WPS220, WPS231, WPS301, WPS414, WPS421, WPS422, WPS505
135135
cheroot/test/test_cli.py: DAR101, DAR201, I001, I005, N802, S101, S108, WPS110, WPS421, WPS431, WPS473
136-
cheroot/test/test_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122
136+
cheroot/test/test_makefile.py: DAR101, DAR201, I004, RST304, S101, WPS110, WPS122, WPS202
137137
cheroot/test/test_wsgi.py: DAR101, DAR301, I001, I004, S101, WPS110, WPS111, WPS117, WPS118, WPS121, WPS210, WPS421, WPS430, WPS432, WPS441, WPS509
138138
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
139139
cheroot/test/test_dispatch.py: DAR101, DAR201, S101, WPS111, WPS121, WPS422, WPS430

cheroot/errors.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import errno
44
import sys
55

6+
from . import _compat
7+
68

79
class MaxSizeExceeded(Exception):
810
"""Exception raised when a client sends more data then allowed under limit.
@@ -66,19 +68,23 @@ def plat_specific_errors(*errnames):
6668

6769

6870
acceptable_sock_shutdown_error_codes = {
71+
errno.EBADF,
6972
errno.ENOTCONN,
7073
errno.EPIPE,
7174
errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3
7275
errno.ECONNRESET, # corresponds to ConnectionResetError in Python 3
76+
*((errno.WSAENOTSOCK,) if _compat.IS_WINDOWS else ()),
7377
}
7478
"""Errors that may happen during the connection close sequence.
7579
80+
* EBADF - raised when operating on a closed socket
7681
* ENOTCONN — client is no longer connected
7782
* EPIPE — write on a pipe while the other end has been closed
7883
* ESHUTDOWN — write on a socket which has been shutdown for writing
7984
* ECONNRESET — connection is reset by the peer, we received a TCP RST packet
8085
8186
Refs:
87+
8288
* https://github.com/cherrypy/cheroot/issues/341#issuecomment-735884889
8389
* https://bugs.python.org/issue30319
8490
* https://bugs.python.org/issue30329
@@ -87,4 +93,8 @@ def plat_specific_errors(*errnames):
8793
* https://docs.microsoft.com/windows/win32/api/winsock/nf-winsock-shutdown
8894
"""
8995

90-
acceptable_sock_shutdown_exceptions = (BrokenPipeError, ConnectionResetError)
96+
97+
acceptable_sock_shutdown_exceptions = (
98+
BrokenPipeError, # Covers EPIPE and ESHUTDOWN
99+
ConnectionResetError, # Covers ECONNRESET
100+
)

cheroot/errors.pyi

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,7 @@ socket_error_eintr: List[int]
1010
socket_errors_to_ignore: List[int]
1111
socket_errors_nonblocking: List[int]
1212
acceptable_sock_shutdown_error_codes: Set[int]
13-
acceptable_sock_shutdown_exceptions: Tuple[Type[Exception], ...]
13+
acceptable_sock_shutdown_exceptions: Tuple[
14+
Type[BrokenPipeError],
15+
Type[ConnectionResetError],
16+
]

cheroot/makefile.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import _pyio as io
55
import socket
66

7+
from . import errors as _errors
8+
79

810
# Write only 16K at a time to sockets
911
SOCK_WRITE_BLOCKSIZE = 16384
@@ -32,8 +34,42 @@ def _flush_unlocked(self):
3234
n = self.raw.write(bytes(self._write_buf))
3335
except io.BlockingIOError as e:
3436
n = e.characters_written
37+
38+
if n == 0:
39+
# If nothing was written we need to break
40+
# to avoid infinte loops
41+
break
42+
3543
del self._write_buf[:n]
3644

45+
def close(self):
46+
"""
47+
Close the stream and its underlying file object.
48+
49+
This method is designed to be idempotent (it can be called multiple
50+
times without side effects). It gracefully handles a race condition
51+
where the underlying socket may have already been closed by the remote
52+
client or another thread.
53+
54+
A :exc:`ConnectionError` or :exc:`OSError` with
55+
:data:`~errno.EBADF` or :data:`~errno.ENOTCONN` is caught
56+
and ignored, as these indicate a normal, expected connection teardown.
57+
Other exceptions are re-raised.
58+
"""
59+
# pylint incorrectly flags inherited self.closed property as constant
60+
if self.closed: # pylint: disable=using-constant-test
61+
return
62+
63+
try:
64+
super().close()
65+
except ConnectionError:
66+
return
67+
except OSError as err:
68+
# Handle EBADF and other acceptable socket shutdown errors
69+
if err.errno in _errors.acceptable_sock_shutdown_error_codes:
70+
return
71+
raise
72+
3773

3874
class StreamReader(io.BufferedReader):
3975
"""Socket stream reader."""

cheroot/ssl/pyopenssl.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@
7777
from . import Adapter
7878

7979

80+
81+
8082
class SSLFileobjectMixin:
8183
"""Base mixin for a TLS socket stream."""
8284

@@ -270,6 +272,7 @@ def __init__(self, *args):
270272
self._lock = threading.RLock()
271273

272274

275+
273276
class pyOpenSSLAdapter(Adapter):
274277
"""A wrapper for integrating :doc:`pyOpenSSL <pyopenssl:index>`."""
275278

cheroot/test/test_makefile.py

Lines changed: 120 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,16 @@
11
"""Tests for :py:mod:`cheroot.makefile`."""
22

3+
import errno
4+
import io
5+
import math
6+
7+
import pytest
8+
39
from cheroot import makefile
410

511

612
class MockSocket:
7-
"""A mock socket."""
13+
"""A mock socket for emulating buffered I/O."""
814

915
def __init__(self):
1016
"""Initialize :py:class:`MockSocket`."""
@@ -51,3 +57,116 @@ def test_bytes_written():
5157
wfile = makefile.MakeFile(sock, 'w')
5258
wfile.write(b'bar')
5359
assert wfile.bytes_written == 3
60+
61+
62+
def test_close_is_idempotent():
63+
"""Test that double ``close()`` does not error out."""
64+
raw_buffer = io.BytesIO()
65+
buffered_writer = makefile.BufferedWriter(raw_buffer)
66+
67+
# Should not raise any exceptions
68+
buffered_writer.close()
69+
assert buffered_writer.closed
70+
71+
buffered_writer.close() # Second call should be safe
72+
assert buffered_writer.closed
73+
74+
75+
def test_close_handles_already_closed_buffer():
76+
"""Test that ``close()`` handles already closed underlying buffer."""
77+
raw_buffer = io.BytesIO()
78+
buffered_writer = makefile.BufferedWriter(raw_buffer)
79+
80+
# Close the underlying buffer first
81+
raw_buffer.close()
82+
83+
# This should not raise an exception
84+
assert raw_buffer.closed
85+
assert buffered_writer.closed
86+
87+
88+
def test_flush_unlocked_handles_blocking_io_error(mock_buffer_writer, mocker):
89+
"""
90+
Test that a BlockingIOError is handled correctly.
91+
92+
We extracting characters_written,
93+
and execution continues without raising the error.
94+
"""
95+
# 1. Create a mock object to replace the real 'write' method
96+
mock_write_method = mocker.Mock()
97+
98+
# 2. Set the side effect on the mock object
99+
err = io.BlockingIOError(errno.EAGAIN, 'Resource temporarily unavailable')
100+
err.characters_written = 5
101+
mock_write_method.side_effect = err
102+
103+
# 3. Use mocker.patch.object to replace the 'write' method
104+
# with mock_write_method
105+
mocker.patch.object(mock_buffer_writer.raw, 'write', new=mock_write_method)
106+
107+
# Check the initial state of the buffer
108+
initial_len = len(mock_buffer_writer._write_buf)
109+
110+
# 4. Execute the code
111+
try:
112+
mock_buffer_writer._flush_unlocked()
113+
except Exception as exc:
114+
pytest.fail(f'Unexpected exception raised: {type(exc).__name__}')
115+
116+
# 5. Verify the side-effect (buffer should be empty)
117+
assert len(mock_buffer_writer._write_buf) == 0
118+
119+
# 6 Check mock calls (Logic/Mechanism)
120+
# The number of calls should be
121+
# initial_len / bytes_written_per_call
122+
expected_calls = math.ceil(initial_len / 5)
123+
assert mock_write_method.call_count == expected_calls
124+
125+
126+
class MockRawSocket:
127+
"""
128+
A mock raw socket for emulating low level unbuffered I/O.
129+
130+
We use this mock with ``io.BufferedWriter``, which accesses it via
131+
the ``.raw`` attribute.
132+
"""
133+
134+
def __init__(self, *args, **kwargs):
135+
"""Initialize :py:class:`MockRawSocket`."""
136+
# 1. Call the parent's init to set up self.messages
137+
super().__init__(*args, **kwargs)
138+
139+
# 2. Rquired by the io.BufferedWriter base class
140+
self._is_closed = False
141+
142+
def write(self, message):
143+
"""Emulate ``io.RawIOBase write``."""
144+
# Use the underlying send method implemented in MockSocket
145+
return self.send(message)
146+
147+
def writable(self):
148+
"""Indicate that the raw stream supports writing."""
149+
return True
150+
151+
def send(self, message):
152+
"""Emulate a send."""
153+
return len(message)
154+
155+
def close(self):
156+
"""Emulate close."""
157+
self._is_closed = True
158+
159+
@property
160+
def closed(self):
161+
"""Emulate the required ``closed`` property."""
162+
return self._is_closed
163+
164+
165+
@pytest.fixture
166+
def mock_buffer_writer():
167+
"""Fixture to create a BufferedWriter instance with a mock raw socket."""
168+
# Create a BufferedWriter instance with a buffer that has content
169+
# to ensure _flush_unlocked attempts to write.
170+
writer = makefile.BufferedWriter(MockRawSocket())
171+
writer._write_buf = bytearray(b'data to flush')
172+
return writer
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Socket I/O is now resilient to race conditions happening during connection teardown
2+
due to sockets dying independently or being closed externally.
3+
4+
-- by :user:`julianz-`

docs/spelling_wordlist.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ subpackages
6565
symlinked
6666
syscall
6767
systemd
68+
teardown
6869
threadpool
6970
Tidelift
7071
TLS

0 commit comments

Comments
 (0)