Refactor _from server socket by julianz- · Pull Request #820 · cherrypy/cheroot

julianz- · 2026-04-07T21:28:48Z

This function in connections.py had become highly nested and complex, triggering linter warnings (C901, WPS505). Decomposed the logic into several smaller private methods.

❓ What kind of change does this PR introduce?

📋 What is the related issue number (starting with #)

Resolves #

❓ What is the current behavior? (You can also link to an open issue here)

❓ What is the new behavior (if this is a feature change)?
None

📋 Other information:
📋 Contribution checklist:

(If you're a first-timer, check out
[this guide on making great pull requests][making a lovely PR])

Will squash on approval

read-the-docs-community · 2026-04-07T21:29:45Z

Documentation build overview

📚 cheroot | 🛠️ Build #33224512 | 📁 Comparing 2b63a58 against latest (18f4ba8)

🔍 Preview build

8 files changed · ± 8 modified

± Modified

codecov · 2026-04-07T21:32:08Z

Codecov Report

❌ Patch coverage is 98.42520% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.76%. Comparing base (2ffb0ba) to head (2b63a58).
⚠️ Report is 5 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #820      +/-   ##
==========================================
+ Coverage   78.32%   78.76%   +0.44%     
==========================================
  Files          41       41              
  Lines        4788     4875      +87     
  Branches      547      555       +8     
==========================================
+ Hits         3750     3840      +90     
+ Misses        900      896       -4     
- Partials      138      139       +1

This is the first in a sequence of refactors for this function. This step moves the initialization of the remote address to a separate helper function

Moves the check on ignorable errors into a separate function.

Puts the wrapping call into its own function.

the try hamdler had multiple statements

The accept() call is now handled in _accept_conn()

Helper function for security marks the socket fd as non-inheritable and sets the timeout.

Using helper functions this function now just has two main steps. First, socket creation. Second, connection configuration.

Added to deal with comment in _setup_conn_addr()

Added as a contrib.rst.

These tests cover code that was previous not covered.

Update expected SSL error substrings to match the format returned by Python 3.15 on Windows Server 2025 runners. This unblocks CI coverage reports.

Better coverage and refactored some previous ones

avinashkamat48

In _wrap_socket_for_tls, the FatalSSLAlert branch now returns (None, {}) immediately, so the cleanup block below never closes raw_socket for that error path. The old _from_server_socket path simply returned, but with this refactor the helper comment says both TLS failure paths should close the socket. The NoSSLError branch reaches the close block, while FatalSSLAlert leaks the accepted socket unless the adapter already closed it. Could you either close before returning in that branch or move the common cleanup into a finally/single exit path?

webknjaz

newer per-commit comments

webknjaz · 2026-06-19T13:34:56Z

            with _cm.suppress(OSError):
                conn.close()

+    def _setup_conn_addr(self, conn, s, addr):


Let's use full words for variable names (things like i/j/k/s aren't really descriptive, nor are the shortened words). It'd also be a good idea to settle for all the args being keyword-only or positional-only (seems like in this case positional args of different types make up bad DX).

webknjaz · 2026-06-19T13:45:05Z


+    def _ignore_socket_oserror(self, exc):
+        if self.server.stats['Enabled']:
+            self.server.stats['Socket Errors'] += 1


This probably belongs outside a function saying "ignore". Though, maybe the helper could be renamed since it doesn't actually ignore anything but checks for expected exceptions.

How about something like def _is_suppressable_exc(exc: OSError, /) -> bool:? OTOH, could you check if specific OSError subclasses aren't being raised? Maybe, we could replace checking the low-level codes with suppressing the corresponding specific exceptions like InterruptedError and similar?

Agreed, though I prefer the slightly more intuitive _is_ignorable_socket_error() as the name. I added the / but since none of the other functions have type annotations I left those out — maybe that's better done in a separate PR?

webknjaz · 2026-06-19T13:47:00Z

+
    def _from_server_socket(self, server_socket):  # noqa: C901  # FIXME
        try:
            s, addr = server_socket.accept()


I wonder why can't we wrap this line with a suppression of a single exception instead of handling a bunch of errors that could come from a lot of arbitrary places within the try-block..

Refactored to handle two different types of exception - _NoConnectionAvailable, ConnectionError. We keep this separation so that transient accept failures (no connection ready) are kept separate from TLS handshake failures.

webknjaz · 2026-06-19T13:50:09Z

        conn.remote_port = addr[1]

    def _ignore_socket_oserror(self, exc):
+        """Determine if an OSError during socket operations should be ignored."""


"determine" is another clue that the helper probably shouldn't say "ignore".

webknjaz · 2026-06-19T13:54:56Z

-                    )
-                    self._send_bad_request_plain_http_error(s)
+                s, ssl_env = self._wrap_socket_for_tls(s, addr)
+                if not s:


Why can't we communicate this through raising a unified exception (ConnectionError? RuntimeError?) instead of relying on turning one of the returned tuple values into a bool for deciding to bail?

Refactored along those lines

webknjaz · 2026-06-19T14:15:47Z

+
+def _raise_eintr(*args, **kwargs):
+    """Raise an interrupt error."""
+    raise OSError(errno.EINTR, 'Interrupted system call')


Wouldn't this be https://docs.python.org/3/library/exceptions.html#InterruptedError IRL?

webknjaz · 2026-06-19T14:19:08Z

+    actual_client = scenario.client_s or fake_socket
+
+    # Mock the server socket to return our scenario-specific data
+    fake_server_socket = SimpleNamespace(


the first half could probably go into a fixture with the rest spread across two tests since the logic here is getting nested and tests should be straightforward.

This may have been addressed by subsequent refactoring — the tests have changed significantly since this comment was made. Could you take another look?

webknjaz · 2026-06-19T14:20:16Z

In general, we should prefer e2e tests since having a lot of mocks will make them tightly coupled with the internals + may make them less reliable.

Agree in principle, although some of these scenarios (TLS handshake failures, transient socket errors) are difficult to trigger reliably in e2e tests without accessing the internals directly.

webknjaz · 2026-06-19T14:22:54Z

-@pytest.fixture
-def fake_socket():
-    """Provide a basic mock socket."""
+@contextlib.contextmanager


this could be a fixture instead

webknjaz · 2026-06-19T14:25:57Z

+This function had become highly nested and complex, triggering
+linter warnings (C901, WPS505). Decomposed the logic into
+several smaller private methods.
+-- by :user:`julianz-`


Suggested change

-- by :user:`julianz-`

-- by :user:`julianz-`

The FatalSSLAlert branch returned early, bypassing the raw_socket.close() cleanup block. Both exception paths now fall through to the shared close.

julianz- · 2026-06-19T19:48:22Z

@avinashkamat48 Thanks for the catch. I have fixed _wrap_socket_for_tls() so that both FatalSSLAlert and NoSSLError now fall through to the shared close block.

The new name better reflects that the method returns a bool rather than performing an action.

Instead of returning (None, None, None, None) on failure, raise _NoConnectionAvailable or ConnectionError to communicate failure explicitly.

Rename s to sock and mf to makefile throughout the private socket helpers, and add positional-only / markers consistent with other methods in the class.

InterruptedError is the specific OSError subclass Python raises for EINTR, making the test more realistic.

fileno() is never called on the server socket when accept() raises, so the pipe file descriptor and its fixture were unnecessary.

socket.timeout has been a subclass of OSError since Python 3.3 and a deprecated alias of TimeoutError since Python 3.10.

Add blank line before the attribution line.

Describe the return tuple in plain prose to avoid abbreviations that the spell checker flags.

for more information, see https://pre-commit.ci

psf-chronographer Bot added the bot:chronographer:provided A mark meaning that a new change log entry is present within the patch. label Apr 7, 2026

github-advanced-security AI found potential problems Apr 7, 2026

View reviewed changes

Comment thread cheroot/connections.py Fixed

julianz- force-pushed the refactor__from_server_socket branch 4 times, most recently from 1f86831 to b2f990a Compare April 8, 2026 04:38

julianz- added 10 commits April 9, 2026 11:57

Refactor of _from_server_socket()

9aac18d

This is the first in a sequence of refactors for this function. This step moves the initialization of the remote address to a separate helper function

Simply the socket OS error handling

b469389

Moves the check on ignorable errors into a separate function.

Move the wrap() operation into a separate helper

4341144

Puts the wrapping call into its own function.

refactor _from_server_socket exception handler

0f28843

the try hamdler had multiple statements

Move socket acceptance into separate helper

526c291

The accept() call is now handled in _accept_conn()

Add socket configuration helper

63a63ad

Helper function for security marks the socket fd as non-inheritable and sets the timeout.

Simplify _from_server_socket into two steps

45b26ef

Using helper functions this function now just has two main steps. First, socket creation. Second, connection configuration.

Added IPv4 and IPv6 to spell check dicitonary

9524853

Added to deal with comment in _setup_conn_addr()

Added towncrier statement for whole refactor

3b39b17

Added as a contrib.rst.

Added tests for refactored code to improve coverage

de7bee4

These tests cover code that was previous not covered.

julianz- force-pushed the refactor__from_server_socket branch 3 times, most recently from 2081bf2 to ddd2b92 Compare April 13, 2026 03:31

julianz- added 2 commits April 12, 2026 20:37

Fix test_tls_client_auth for Windows 2025

be92e4b

Update expected SSL error substrings to match the format returned by Python 3.15 on Windows Server 2025 runners. This unblocks CI coverage reports.

Updated tests for from_server_socket

57031ff

Better coverage and refactored some previous ones

julianz- force-pushed the refactor__from_server_socket branch from ddd2b92 to 57031ff Compare April 13, 2026 03:37

avinashkamat48 reviewed Jun 13, 2026

View reviewed changes

avinashkamat48 mentioned this pull request Jun 15, 2026

Refactor _from_server_socket() #819

Closed

16 tasks

webknjaz reviewed Jun 19, 2026

View reviewed changes

Fix socket leak in _wrap_socket_for_tls on FatalSSLAlert

6892f6b

The FatalSSLAlert branch returned early, bypassing the raw_socket.close() cleanup block. Both exception paths now fall through to the shared close.

julianz- force-pushed the refactor__from_server_socket branch from 62a0316 to 9078584 Compare June 19, 2026 23:47

julianz- added 7 commits June 19, 2026 16:59

Rename _ignore_socket_oserror to _is_ignorable_socket_error

5a26b85

The new name better reflects that the method returns a bool rather than performing an action.

Replace sentinel tuple returns with exceptions in _prepare_socket

046a870

Instead of returning (None, None, None, None) on failure, raise _NoConnectionAvailable or ConnectionError to communicate failure explicitly.

Use full variable names and positional-only args in socket methods

0348719

Rename s to sock and mf to makefile throughout the private socket helpers, and add positional-only / markers consistent with other methods in the class.

Use InterruptedError instead of OSError(EINTR) in transport error test

6a2809c

InterruptedError is the specific OSError subclass Python raises for EINTR, making the test more realistic.

Simplify transport error test by removing unnecessary pipe_fd fixture

809dd22

fileno() is never called on the server socket when accept() raises, so the pipe file descriptor and its fixture were unnecessary.

Document socket.timeout as OSError subclass and TimeoutError alias

01fde84

socket.timeout has been a subclass of OSError since Python 3.3 and a deprecated alias of TimeoutError since Python 3.10.

Fix changelog fragment formatting

050a198

Add blank line before the attribution line.

julianz- force-pushed the refactor__from_server_socket branch from 2b6804b to 4ab7e86 Compare June 20, 2026 00:02

Rephrase _prepare_socket docstring to avoid spell check failures

96aa39f

Describe the return tuple in plain prose to avoid abbreviations that the spell checker flags.

julianz- force-pushed the refactor__from_server_socket branch from 4ab7e86 to 96aa39f Compare June 20, 2026 00:04

[pre-commit.ci] auto fixes from pre-commit.com hooks

2b63a58

for more information, see https://pre-commit.ci

Uh oh!

Conversation

julianz- commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

read-the-docs-community Bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

Uh oh!

codecov Bot commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

avinashkamat48 left a comment

Choose a reason for hiding this comment

Uh oh!

webknjaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianz- Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianz- commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

julianz- commented Apr 7, 2026 •

edited

Loading

read-the-docs-community Bot commented Apr 7, 2026 •

edited

Loading

codecov Bot commented Apr 7, 2026 •

edited

Loading

julianz- Jun 19, 2026 •

edited

Loading