Refactor _from_server_socket() by julianz- · Pull Request #819 · cherrypy/cheroot

julianz- · 2026-04-07T07:12:51Z

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

_wrap_tls_socket()
_create_conn()
_handle_socket_error()
_setup_conn_addr()

❓ 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)?

📋 Other information:

📋 Contribution checklist:

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

codecov · 2026-04-07T07:16:20Z

Codecov Report

❌ Patch coverage is 64.44444% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.20%. Comparing base (2ffb0ba) to head (4d67561).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #819      +/-   ##
==========================================
- Coverage   78.32%   78.20%   -0.12%     
==========================================
  Files          41       41              
  Lines        4788     4803      +15     
  Branches      547      549       +2     
==========================================
+ Hits         3750     3756       +6     
- Misses        900      907       +7     
- Partials      138      140       +2

read-the-docs-community · 2026-04-07T07:17:06Z

Documentation build overview

📚 cheroot | 🛠️ Build #32145455 | 📁 Comparing 4d67561 against latest (2ffb0ba)

🔍 Preview build

Show files changed (6 files in total): 📝 6 modified | ➕ 0 added | ➖ 0 deleted

File	Status
genindex/index.html	📝 modified
history/index.html	📝 modified
pkg/cheroot/index.html	📝 modified
pkg/cheroot.connections/index.html	📝 modified
pkg/cheroot.server/index.html	📝 modified
pkg/cheroot.ssl.pyopenssl/index.html	📝 modified

This function had become highly nested and complex, triggering linter warnings (C901, WPS505). Decomposed the logic into several smaller private methods: - ``_wrap_tls_socket()`` - ``_create_conn()`` - ``_handle_socket_error()`` - ``_setup_conn_addr()``

avinashkamat48

This looks superseded by #820: it refactors the same _from_server_socket() path, is from the same author, and the newer PR has additional commits/tests while this one still has both test checklist items unchecked. To avoid reviewing/merging two divergent versions of the same refactor, it would be cleaner to close this older PR and keep the discussion on #820.

webknjaz

old unsubmitted comments

webknjaz · 2026-04-07T15:55:14Z

                conn.close()

-    def _from_server_socket(self, server_socket):  # noqa: C901  # FIXME
+    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-04-07T15:56:53Z

+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-`

webknjaz · 2026-04-07T15:57:43Z

@@ -0,0 +1,5 @@
+Refactored ``_from_server_socket()`` in :py:class:`~cheroot.connections.ConnectionManager`.
+This function had become highly nested and complex, triggering
+linter warnings (C901, WPS505). Decomposed the logic into


It's probably better to spell out what the violations are instead of using the trampoline codes — people don't tend to memorize what these numbers are anyway.

webknjaz · 2026-04-07T15:58:05Z

+        if self.server.stats['Enabled']:
+            self.server.stats['Socket Errors'] += 1
+
+        if s:


Perhaps, the if-block would be nicer inside the CM?

webknjaz · 2026-04-07T16:40:32Z

+
+        err_code = ex.args[0] if ex.args else None
+        ignored_groups = (
+            errors.socket_error_eintr,


If you expand all the iterables like this

Suggested change

errors.socket_error_eintr,

*errors.socket_error_eintr,

then you won't need that double-loop generation expression in the if-block.

webknjaz · 2026-04-07T16:41:03Z

+            # Our socket was closed.
+            # See https://github.com/cherrypy/cherrypy/issues/686.
+        )
+        if any(err_code in group for group in ignored_groups):


Alternatively, there's itertools.chain() for unwrapping nested things.

webknjaz · 2026-04-07T18:18:53Z

+            # or if the handshake fails it may exist but
+            # will need to be closed.
+            # pass it over to error handler if it exists
+            return self._handle_socket_error(ex, locals().get('s'))


The dynamic locals lookup seems weird here — it doesn't make sense to me.

webknjaz · 2026-04-07T18:19:50Z

            s, addr = server_socket.accept()
            if self.server.stats['Enabled']:
                self.server.stats['Accepts'] += 1
+


Let's avoid making formatting changes in functional PRs.

webknjaz · 2026-04-07T18:23:12Z

-                # See https://github.com/cherrypy/cherrypy/issues/686.
-                return None
-            raise
+            # if socket.accept() fails s may not be defined


So why not initialize it as None earlier, then?

julianz- force-pushed the refactor-from-server-socket branch from ed64521 to 21e36e0 Compare April 7, 2026 07:22

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

julianz- force-pushed the refactor-from-server-socket branch 2 times, most recently from 7f77695 to 53c228c Compare April 7, 2026 07:38

Refactor _from_server_socket()

4d67561

This function had become highly nested and complex, triggering linter warnings (C901, WPS505). Decomposed the logic into several smaller private methods: - ``_wrap_tls_socket()`` - ``_create_conn()`` - ``_handle_socket_error()`` - ``_setup_conn_addr()``

julianz- force-pushed the refactor-from-server-socket branch from 53c228c to 4d67561 Compare April 7, 2026 07:49

julianz- marked this pull request as draft April 7, 2026 20:58

avinashkamat48 reviewed Jun 15, 2026

View reviewed changes

webknjaz reviewed Jun 19, 2026

View reviewed changes

julianz- closed this Jun 19, 2026

Uh oh!

Conversation

julianz- commented Apr 7, 2026

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Apr 7, 2026 •

edited

Loading

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

edited

Loading