-
-
Notifications
You must be signed in to change notification settings - Fork 103
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 1 commit
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,27 @@ 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 _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 +356,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 | ||
|
|
||
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