Rewrite WerkzeugServer for better IPv4/v6 handling #1449
Conversation
There was a problem hiding this comment.
Thanks for doing a deep dive and fixing the issue once and for all!
Seems like a test is failing:
ERROR: testSpecifiedHost (__main__.WerkzeugServerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/.bazel-output-
...
OSError: [Errno 99] Cannot assign requested address
tensorboard/program.py
Outdated
|
|
||
| def serve_forever(self): | ||
| self._server.serve_forever() | ||
| def _get_wildcard_address(self, port): |
There was a problem hiding this comment.
can you rename this method to _get_wildcard_host?
It seems like word address has been conflated.
>>> wildcard_addrs = socket.getaddrinfo(
None, port, socket.AF_UNSPEC, socket.SOCK_STREAM,
socket.IPPROTO_TCP, socket.AI_PASSIVE)
<<< [(30, 1, 6, ('::', 6006, 0, 0))]
from what I can tell, the sockaddr in L318 refers to <host, port, ?, ?> tuple and in L322, we seem to be returning only the host part.
There was a problem hiding this comment.
Hmm, I'd prefer to stick with "address". IMO "wildcard host" doesn't really make sense because 0.0.0.0 isn't a hostname and no host actually has that IP address.
Unfortunately this terminology is just a bit confusing - "socket address" typically does include the IP address as well as a port: https://en.wikipedia.org/wiki/Network_socket#Socket_addresses
I've reworded the rest of the function body though to be a bit clearer, so now I name the return value "addrinfos" instead of "wildcard_addrs" and added a comment to clarify the sockaddr part as a socket address.
tensorboard/program.py
Outdated
| msg = ( | ||
| 'TensorBoard attempted to bind to port %d, but it was already in use' | ||
| % port) | ||
| raise |
There was a problem hiding this comment.
might be in-familiarity but what does this do and how does it relate to L298?
There was a problem hiding this comment.
Revised this to be clearer about the control flow
tensorboard/program.py
Outdated
| msg = ( | ||
| 'TensorBoard attempted to bind to port %d, but it was already in use' | ||
| % port) | ||
| raise |
There was a problem hiding this comment.
Revised this to be clearer about the control flow
tensorboard/program.py
Outdated
|
|
||
| def serve_forever(self): | ||
| self._server.serve_forever() | ||
| def _get_wildcard_address(self, port): |
There was a problem hiding this comment.
Hmm, I'd prefer to stick with "address". IMO "wildcard host" doesn't really make sense because 0.0.0.0 isn't a hostname and no host actually has that IP address.
Unfortunately this terminology is just a bit confusing - "socket address" typically does include the IP address as well as a port: https://en.wikipedia.org/wiki/Network_socket#Socket_addresses
I've reworded the rest of the function body though to be a bit clearer, so now I name the return value "addrinfos" instead of "wildcard_addrs" and added a comment to clarify the sockaddr part as a socket address.
|
LGTM |
This rewrites the WerkzeugServer wrapper introduced in #1409 to eliminate some issues with the old approach to IPv4/v6 handling and in particular fix #1320. Gory details are in #1320 (comment)
This change also streamlines the class so that it now directly inherits from Werkzeug's ThreadedWSGIServer class, which gives us a
serve_forever()implementation for free and cleans up thehandle_error()behavior so it's an overridden method instead of monkey-patched.Fixes #1320