-
-
Notifications
You must be signed in to change notification settings - Fork 100
Refactor _from server socket #820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
9aac18d
b469389
4341144
0f28843
526c291
63a63ad
45b26ef
9524853
3b39b17
de7bee4
be92e4b
57031ff
6892f6b
5a26b85
046a870
0348719
6a2809c
809dd22
01fde84
050a198
96aa39f
2b63a58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -295,6 +295,44 @@ def _remove_invalid_sockets(self): | |
| with _cm.suppress(OSError): | ||
| conn.close() | ||
|
|
||
| def _setup_conn_addr(self, conn, s, addr): | ||
| """Configure remote address and port for the connection. | ||
|
|
||
| Populates the connection object with remote address metadata. | ||
| If the address is unavailable following the handshake, this | ||
| method detects the socket's address family (IPv4/IPv6) | ||
| and provides an appropriate 'any' address placeholder. | ||
| """ | ||
| # 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] | ||
|
|
||
| def _ignore_socket_oserror(self, exc): | ||
| if self.server.stats['Enabled']: | ||
| self.server.stats['Socket Errors'] += 1 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| err_code = exc.args[0] | ||
|
|
||
| # EINTR should occur when a signal is received during accept(); | ||
| # retry the call. See https://github.com/cherrypy/cherrypy/issues/707. | ||
| is_eintr = err_code in errors.socket_error_eintr | ||
|
|
||
| # Just try again. See https://github.com/cherrypy/cherrypy/issues/479. | ||
| is_nonblocking = err_code in errors.socket_errors_nonblocking | ||
|
|
||
| # Our socket was closed. See https://github.com/cherrypy/cherrypy/issues/686. | ||
| is_ignored = err_code in errors.socket_errors_to_ignore | ||
|
|
||
| return is_eintr or is_nonblocking or is_ignored | ||
|
|
||
| def _from_server_socket(self, server_socket): # noqa: C901 # FIXME | ||
| try: | ||
| s, addr = server_socket.accept() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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..
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
@@ -335,18 +373,7 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME | |
| 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] | ||
| self._setup_conn_addr(conn, s, addr) | ||
|
|
||
| conn.ssl_env = ssl_env | ||
| return conn | ||
|
|
@@ -356,24 +383,8 @@ def _from_server_socket(self, server_socket): # noqa: C901 # FIXME | |
| # notice keyboard interrupts on Win32, which don't interrupt | ||
| # 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. | ||
| except OSError as exc: | ||
| if self._ignore_socket_oserror(exc): | ||
| return None | ||
| raise | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done