Skip to content

Commit 775d221

Browse files
committed
Fix socket teardown errors and stub file issues
- Fixed socket teardown errors that were thrown during connection cleanup - Added missing __all__ declarations to server.pyi and threadpool.pyi - Removed incorrect WIN_SOCKET_NOT_OPEN definition from makefile.pyi - Resolved stubtest failures across multiple modules
1 parent 7eced3d commit 775d221

File tree

15 files changed

+213
-25
lines changed

15 files changed

+213
-25
lines changed

.github/workflows/ci-cd.yml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ on:
1313
- patchback/backports/** # Patchback always creates PRs
1414
- pre-commit-ci-update-config # pre-commit.ci always creates a PR
1515
pull_request:
16-
ignore-paths: # changes to the cron workflow are triggered through it
16+
paths-ignore: # changes to the cron workflow are triggered through it
1717
- .github/workflows/scheduled-runs.yml
1818
workflow_call: # a way to embed the main tests
1919
workflow_dispatch:
@@ -83,6 +83,7 @@ env:
8383
PYTHONIOENCODING
8484
PYTHONLEGACYWINDOWSSTDIO
8585
PYTHONUTF8
86+
PYTHONWARNINGS: "ignore::ResourceWarning:sqlite3"
8687
TOX_VERSION: tox < 4.12
8788
UPSTREAM_REPOSITORY_ID: >-
8889
16620627

cheroot/connections.pyi

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
from typing import Any
22

3+
IS_WINDOWS: bool
4+
35
def prevent_socket_inheritance(sock) -> None: ...
46

57
class _ThreadsafeSelector:

cheroot/errors.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@
44
import sys
55

66

7+
try:
8+
from OpenSSL.SSL import SysCallError as _OpenSSL_SysCallError
9+
except ImportError:
10+
_OpenSSL_SysCallError = None
11+
12+
713
class MaxSizeExceeded(Exception):
814
"""Exception raised when a client sends more data then allowed under limit.
915
@@ -66,6 +72,7 @@ def plat_specific_errors(*errnames):
6672

6773

6874
acceptable_sock_shutdown_error_codes = {
75+
errno.EBADF,
6976
errno.ENOTCONN,
7077
errno.EPIPE,
7178
errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3
@@ -87,4 +94,8 @@ def plat_specific_errors(*errnames):
8794
* https://docs.microsoft.com/windows/win32/api/winsock/nf-winsock-shutdown
8895
"""
8996

90-
acceptable_sock_shutdown_exceptions = (BrokenPipeError, ConnectionResetError)
97+
acceptable_sock_shutdown_exceptions = (
98+
BrokenPipeError,
99+
ConnectionResetError,
100+
_OpenSSL_SysCallError,
101+
)

cheroot/makefile.py

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,30 @@
22

33
# prefer slower Python-based io module
44
import _pyio as io
5+
import errno
56
import socket
7+
import sys
68

9+
from OpenSSL.SSL import SysCallError
10+
11+
12+
# Define a variable to hold the platform-specific "not a socket" error.
13+
_not_a_socket_err = None
14+
15+
if sys.platform == 'win32':
16+
# On Windows, try to get the named constant from the socket module.
17+
# If that fails, fall back to the known numeric value.
18+
try:
19+
_not_a_socket_err = socket.WSAENOTSOCK
20+
except AttributeError:
21+
_not_a_socket_err = 10038
22+
else:
23+
# On other platforms, the relevant error is EBADF (Bad file descriptor),
24+
# which is already in your list of handled errors.
25+
pass
26+
27+
# Expose the error constant for use in the module's public API if needed.
28+
WSAENOTSOCK = _not_a_socket_err
729

830
# Write only 16K at a time to sockets
931
SOCK_WRITE_BLOCKSIZE = 16384
@@ -30,10 +52,59 @@ def _flush_unlocked(self):
3052
# ssl sockets only except 'bytes', not bytearrays
3153
# so perhaps we should conditionally wrap this for perf?
3254
n = self.raw.write(bytes(self._write_buf))
33-
except io.BlockingIOError as e:
34-
n = e.characters_written
55+
except (io.BlockingIOError, OSError, SysCallError) as e:
56+
# Check for a different error attribute depending
57+
# on the exception type
58+
if isinstance(e, io.BlockingIOError):
59+
n = e.characters_written
60+
else:
61+
error_code = (
62+
e.errno if isinstance(e, OSError) else e.args[0]
63+
)
64+
if error_code in {
65+
errno.EBADF,
66+
errno.ENOTCONN,
67+
errno.EPIPE,
68+
WSAENOTSOCK, # Windows-specific error
69+
}:
70+
# The socket is gone, so just ignore this error.
71+
return
72+
raise
73+
else:
74+
# The 'try' block completed without an exception
75+
if n is None:
76+
# This could happen with non-blocking write
77+
# when nothing was written
78+
break
79+
3580
del self._write_buf[:n]
3681

82+
def close(self):
83+
"""
84+
Close the stream and its underlying file object.
85+
86+
This method is designed to be idempotent (it can be called multiple
87+
times without side effects). It gracefully handles a race condition
88+
where the underlying socket may have already been closed by the remote
89+
client or another thread.
90+
91+
A SysCallError or OSError with errno.EBADF or errno.ENOTCONN is caught
92+
and ignored, as these indicate a normal, expected connection teardown.
93+
Other exceptions are re-raised.
94+
"""
95+
if self.closed: # pylint: disable=W0125
96+
return
97+
98+
try:
99+
super().close()
100+
except (OSError, SysCallError) as e:
101+
error_code = e.errno if isinstance(e, OSError) else e.args[0]
102+
if error_code in {errno.EBADF, errno.ENOTCONN}:
103+
# The socket is already closed, which is expected during
104+
# a race condition.
105+
return
106+
raise
107+
37108

38109
class StreamReader(io.BufferedReader):
39110
"""Socket stream reader."""

cheroot/makefile.pyi

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,14 @@
11
import io
2+
import sys
3+
from typing import Optional
4+
5+
WSAENOTSOCK: Optional[int]
26

37
SOCK_WRITE_BLOCKSIZE: int
48

9+
if sys.platform == 'win32':
10+
WIN_SOCKET_NOT_OPEN: Optional[int]
11+
512
class BufferedWriter(io.BufferedWriter):
613
def write(self, b): ...
714

cheroot/server.py

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767

6868
import contextlib
6969
import email.utils
70+
import errno
7071
import io
7172
import logging
7273
import os
@@ -81,6 +82,8 @@
8182
import urllib.parse
8283
from functools import lru_cache
8384

85+
from OpenSSL.SSL import SysCallError
86+
8487
from . import __version__, connections, errors
8588
from ._compat import IS_PPC, bton
8689
from .makefile import MakeFile, StreamWriter
@@ -1186,12 +1189,25 @@ def ensure_headers_sent(self):
11861189

11871190
def write(self, chunk):
11881191
"""Write unbuffered data to the client."""
1189-
if self.chunked_write and chunk:
1190-
chunk_size_hex = hex(len(chunk))[2:].encode('ascii')
1191-
buf = [chunk_size_hex, CRLF, chunk, CRLF]
1192-
self.conn.wfile.write(EMPTY.join(buf))
1193-
else:
1194-
self.conn.wfile.write(chunk)
1192+
try:
1193+
if self.chunked_write and chunk:
1194+
chunk_size_hex = hex(len(chunk))[2:].encode('ascii')
1195+
buf = [chunk_size_hex, CRLF, chunk, CRLF]
1196+
self.conn.wfile.write(EMPTY.join(buf))
1197+
else:
1198+
self.conn.wfile.write(chunk)
1199+
except (SysCallError, ConnectionError, OSError) as e:
1200+
error_code = e.errno if isinstance(e, OSError) else e.args[0]
1201+
if error_code in {
1202+
errno.ECONNRESET,
1203+
errno.EPIPE,
1204+
errno.ENOTCONN,
1205+
errno.EBADF,
1206+
}:
1207+
# The socket is gone, so just ignore this error.
1208+
return
1209+
1210+
raise
11951211

11961212
def send_headers(self): # noqa: C901 # FIXME
11971213
"""Assert, process, and send the HTTP response message-headers.
@@ -1285,7 +1301,27 @@ def send_headers(self): # noqa: C901 # FIXME
12851301
for k, v in self.outheaders:
12861302
buf.append(k + COLON + SPACE + v + CRLF)
12871303
buf.append(CRLF)
1288-
self.conn.wfile.write(EMPTY.join(buf))
1304+
try:
1305+
self.conn.wfile.write(EMPTY.join(buf))
1306+
except (SysCallError, ConnectionError, OSError) as e:
1307+
# We explicitly ignore these errors because they indicate the
1308+
# client has already closed the connection, which is a normal
1309+
# occurrence during a race condition.
1310+
1311+
# The .errno attribute is only available on OSError
1312+
# The .args[0] attribute is available on SysCallError
1313+
# Check for both cases to handle different exception types
1314+
error_code = e.errno if isinstance(e, OSError) else e.args[0]
1315+
if error_code in {
1316+
errno.ECONNRESET,
1317+
errno.EPIPE,
1318+
errno.ENOTCONN,
1319+
errno.EBADF,
1320+
}:
1321+
self.close_connection = True
1322+
self.conn.close()
1323+
return
1324+
raise
12891325

12901326

12911327
class HTTPConnection:
@@ -1541,9 +1577,10 @@ def _close_kernel_socket(self):
15411577
self.socket.shutdown,
15421578
)
15431579

1580+
acceptable_exceptions = errors.acceptable_sock_shutdown_exceptions
15441581
try:
15451582
shutdown(socket.SHUT_RDWR) # actually send a TCP FIN
1546-
except errors.acceptable_sock_shutdown_exceptions:
1583+
except acceptable_exceptions: # pylint: disable=E0712
15471584
pass
15481585
except socket.error as e:
15491586
if e.errno not in errors.acceptable_sock_shutdown_error_codes:

cheroot/server.pyi

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,18 @@
11
from typing import Any
22

3+
__all__ = (
4+
'ChunkedRFile',
5+
'DropUnderscoreHeaderReader',
6+
'Gateway',
7+
'HTTPConnection',
8+
'HTTPRequest',
9+
'HTTPServer',
10+
'HeaderReader',
11+
'KnownLengthRFile',
12+
'SizeCheckWrapper',
13+
'get_ssl_adapter_class',
14+
)
15+
316
class HeaderReader:
417
def __call__(self, rfile, hdict: Any | None = ...): ...
518

cheroot/ssl/builtin.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -286,11 +286,19 @@ def wrap(self, sock):
286286
raise errors.FatalSSLAlert(
287287
*tls_connection_drop_error.args,
288288
) from tls_connection_drop_error
289-
except ssl.SSLError as generic_tls_error:
290-
peer_speaks_plain_http_over_https = (
291-
generic_tls_error.errno == ssl.SSL_ERROR_SSL
292-
and _assert_ssl_exc_contains(generic_tls_error, 'http request')
293-
)
289+
except (
290+
ssl.SSLError,
291+
OSError,
292+
) as generic_tls_error:
293+
# When the client speaks plain HTTP into a TLS-only connection,
294+
# Python's builtin ssl raises an SSLError with `http request`
295+
# in its message. Sometimes, due to a race condition, the socket
296+
# is closed by the time we try to handle the error, resulting in an
297+
# OSError: [Errno 9] Bad file descriptor.
298+
peer_speaks_plain_http_over_https = isinstance(
299+
generic_tls_error,
300+
ssl.SSLError,
301+
) and _assert_ssl_exc_contains(generic_tls_error, 'http request')
294302
if peer_speaks_plain_http_over_https:
295303
reraised_connection_drop_exc_cls = errors.NoSSLError
296304
else:
@@ -299,10 +307,6 @@ def wrap(self, sock):
299307
raise reraised_connection_drop_exc_cls(
300308
*generic_tls_error.args,
301309
) from generic_tls_error
302-
except OSError as tcp_connection_drop_error:
303-
raise errors.FatalSSLAlert(
304-
*tcp_connection_drop_error.args,
305-
) from tcp_connection_drop_error
306310

307311
return s, self.get_environ(s)
308312

cheroot/test/test_conn.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import cheroot.server
1919
from cheroot._compat import IS_CI, IS_MACOS, IS_PYPY, IS_WINDOWS
20+
from cheroot.makefile import BufferedWriter
2021
from cheroot.test import helper, webtest
2122

2223

@@ -1637,3 +1638,33 @@ def test_invalid_selected_connection(test_client, monkeypatch):
16371638
time.sleep(test_client.server_instance.expiration_interval * 2)
16381639
assert faux_select.os_error_triggered
16391640
assert faux_get_map.conn_closed
1641+
1642+
1643+
class TestBufferedWriter:
1644+
"""Test cases for BufferedWriter close() method race condition handling."""
1645+
1646+
def test_close_is_idempotent(self):
1647+
"""Test that close() can be called multiple times safely."""
1648+
import io
1649+
1650+
raw_buffer = io.BytesIO()
1651+
buffered_writer = BufferedWriter(raw_buffer)
1652+
1653+
# Should not raise any exceptions
1654+
buffered_writer.close()
1655+
buffered_writer.close() # Second call should be safe
1656+
1657+
assert buffered_writer.closed
1658+
1659+
def test_close_handles_already_closed_buffer(self):
1660+
"""Test that close() handles already closed underlying buffer."""
1661+
import io
1662+
1663+
raw_buffer = io.BytesIO()
1664+
buffered_writer = BufferedWriter(raw_buffer)
1665+
1666+
# Close the underlying buffer first
1667+
raw_buffer.close()
1668+
1669+
# This should not raise an exception
1670+
buffered_writer.close()

cheroot/workers/threadpool.pyi

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import threading
22
from typing import Any
33

4+
__all__ = ('ThreadPool', 'WorkerThread')
5+
46
class TrueyZero:
57
def __add__(self, other): ...
68
def __radd__(self, other): ...

0 commit comments

Comments
 (0)