From 4d67561c987f0da9c47916055a867e8b6d92c189 Mon Sep 17 00:00:00 2001 From: julianz- <6255571+julianz-@users.noreply.github.com> Date: Mon, 6 Apr 2026 23:06:41 -0700 Subject: [PATCH] Refactor ``_from_server_socket()`` 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()`` --- cheroot/connections.py | 169 +++++++++++++-------- docs/changelog-fragments.d/819.contrib.rst | 5 + 2 files changed, 114 insertions(+), 60 deletions(-) create mode 100644 docs/changelog-fragments.d/819.contrib.rst diff --git a/cheroot/connections.py b/cheroot/connections.py index 98f17e42f5..2520c652b5 100644 --- a/cheroot/connections.py +++ b/cheroot/connections.py @@ -295,61 +295,124 @@ def _remove_invalid_sockets(self): with _cm.suppress(OSError): conn.close() - def _from_server_socket(self, server_socket): # noqa: C901 # FIXME + def _setup_conn_addr(self, conn, s, addr): + """Configure remote address and port for the connection.""" + # Optional values + # Until we do DNS lookups, omit REMOTE_HOST + if addr is None: + # Fallback for sockets that don't return an address on accept + # figure out if AF_INET or AF_INET6. + if len(s.getsockname()) == 2: + # AF_INET + addr = ('0.0.0.0', 0) + else: + # AF_INET6 + addr = ('::', 0) + conn.remote_addr = addr[0] + conn.remote_port = addr[1] + + def _handle_socket_error(self, ex, s=None): + """Handle OSErrors and determine if they should be ignored.""" + if self.server.stats['Enabled']: + self.server.stats['Socket Errors'] += 1 + + if s: + with _cm.suppress(OSError): + s.close() + + err_code = ex.args[0] if ex.args else None + ignored_groups = ( + errors.socket_error_eintr, + # I *think* this is right. EINTR should occur when a signal + # is received during the accept() call; all docs say retry + # the call, and I *think* I'm reading it right that Python + # will then go ahead and poll for and handle the signal + # elsewhere. See + # https://github.com/cherrypy/cherrypy/issues/707. + errors.socket_errors_nonblocking, + # Just try again. See + # https://github.com/cherrypy/cherrypy/issues/479. + errors.socket_errors_to_ignore, + # Our socket was closed. + # See https://github.com/cherrypy/cherrypy/issues/686. + ) + if any(err_code in group for group in ignored_groups): + return + + raise ex + + def _wrap_tls_socket(self, s, addr): + """Handle the TLS wrap and log specific error responses. + + on success returns e.g. (SSLSocket, {'SSL_PROTOCOL': 'TLSv1.3', ...}). + on failure returns None, {} + """ + try: + return self.server.ssl_adapter.wrap(s) + except errors.FatalSSLAlert as tls_connection_drop_error: + self.server.error_log( + f'Client {addr!s} lost — peer dropped the TLS ' + 'connection suddenly, during handshake: ' + f'{tls_connection_drop_error!s}', + ) + except errors.NoSSLError as http_over_https_err: + self.server.error_log( + f'Client {addr!s} attempted to speak plain HTTP into ' + 'a TCP connection configured for TLS-only traffic — ' + 'trying to send back a plain HTTP error response: ' + f'{http_over_https_err!s}', + ) + self._send_bad_request_plain_http_error(s) + + # If we hit either exception, close the socket and signal failure + with _cm.suppress(OSError): + s.close() + return None, {} + + def _create_conn(self, s, addr, ssl_env): + """Build and configure the Connection object.""" + # 1. Determine the makefile type (SSL vs Plain) + mf = MakeFile + if self.server.ssl_adapter is not None: + mf = self.server.ssl_adapter.makefile + + # 2. Re-apply timeout specifically for the new SSLSocket object + if hasattr(s, 'settimeout'): + s.settimeout(self.server.timeout) + + # 3. Create the actual connection object + conn = self.server.ConnectionClass(self.server, s, mf) + conn.ssl_env = ssl_env + + # 4. Configure the remote address/port if it's not a Unix socket + if not isinstance(self.server.bind_addr, (str, bytes)): + self._setup_conn_addr(conn, s, addr) + + return conn + + def _from_server_socket(self, server_socket): + """Coordinate socket acceptance and connection initialization.""" try: s, addr = server_socket.accept() if self.server.stats['Enabled']: self.server.stats['Accepts'] += 1 + prevent_socket_inheritance(s) if hasattr(s, 'settimeout'): s.settimeout(self.server.timeout) - mf = MakeFile ssl_env = {} - # if ssl cert and key are set, we try to be a secure HTTP server if self.server.ssl_adapter is not None: - # FIXME: WPS505 -- too many nested blocks - try: # noqa: WPS505 - s, ssl_env = self.server.ssl_adapter.wrap(s) - except errors.FatalSSLAlert as tls_connection_drop_error: - self.server.error_log( - f'Client {addr!s} lost — peer dropped the TLS ' - 'connection suddenly, during handshake: ' - f'{tls_connection_drop_error!s}', - ) - return None - except errors.NoSSLError as http_over_https_err: - self.server.error_log( - f'Client {addr!s} attempted to speak plain HTTP into ' - 'a TCP connection configured for TLS-only traffic — ' - 'trying to send back a plain HTTP error response: ' - f'{http_over_https_err!s}', - ) - self._send_bad_request_plain_http_error(s) + # try to become a secure server + s, ssl_env = self._wrap_tls_socket(s, addr) + if s is None: return None - mf = self.server.ssl_adapter.makefile - # Re-apply our timeout since we may have a new socket object + + # Re-apply timeout to the new SSLSocket object if hasattr(s, 'settimeout'): s.settimeout(self.server.timeout) - conn = self.server.ConnectionClass(self.server, s, mf) - - if not isinstance(self.server.bind_addr, (str, bytes)): - # optional values - # Until we do DNS lookups, omit REMOTE_HOST - if addr is None: # sometimes this can happen - # figure out if AF_INET or AF_INET6. - if len(s.getsockname()) == 2: - # AF_INET - addr = ('0.0.0.0', 0) - else: - # AF_INET6 - addr = ('::', 0) - conn.remote_addr = addr[0] - conn.remote_port = addr[1] - - conn.ssl_env = ssl_env - return conn + return self._create_conn(s, addr, ssl_env) except socket.timeout: # The only reason for the timeout in start() is so we can @@ -357,25 +420,11 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME # accept() by default return None except OSError as ex: - if self.server.stats['Enabled']: - self.server.stats['Socket Errors'] += 1 - if ex.args[0] in errors.socket_error_eintr: - # I *think* this is right. EINTR should occur when a signal - # is received during the accept() call; all docs say retry - # the call, and I *think* I'm reading it right that Python - # will then go ahead and poll for and handle the signal - # elsewhere. See - # https://github.com/cherrypy/cherrypy/issues/707. - return None - if ex.args[0] in errors.socket_errors_nonblocking: - # Just try again. See - # https://github.com/cherrypy/cherrypy/issues/479. - return None - if ex.args[0] in errors.socket_errors_to_ignore: - # Our socket was closed. - # See https://github.com/cherrypy/cherrypy/issues/686. - return None - raise + # if socket.accept() fails s may not be defined + # 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')) def close(self): """Close all monitored connections.""" diff --git a/docs/changelog-fragments.d/819.contrib.rst b/docs/changelog-fragments.d/819.contrib.rst new file mode 100644 index 0000000000..a7eb61e2b9 --- /dev/null +++ b/docs/changelog-fragments.d/819.contrib.rst @@ -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 +several smaller private methods. +-- by :user:`julianz-`