Skip to content

server: amend #1851’s port-crawling implementation #1858

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.

Sign up for GitHub

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

Merged
wchargin merged 4 commits into from
Feb 16, 2019
Merged

server: amend #1851’s port-crawling implementation #1858

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9f55df
review: TODO: call super initializers linearly
wchargin Feb 16, 2019
9f17457
review: use hex constants consistently
wchargin Feb 16, 2019
b1116c0
review: explode tuple construction
wchargin Feb 16, 2019
be66860
review: recompute wildcard address at each port
wchargin Feb 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions tensorboard/program.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,33 +352,37 @@ def __init__(self, wsgi_app, flags):

# base_port: what's the first port to which we should try to bind?
# should_scan: if that fails, shall we try additional ports?
(base_port, should_scan) = (
(flags.port, False)
if flags.port is not None
else (core_plugin.DEFAULT_PORT, True)
)
# max_attempts: how many ports shall we try?
should_scan = flags.port is None
base_port = core_plugin.DEFAULT_PORT if flags.port is None else flags.port
max_attempts = 10 if should_scan else 1

if base_port > 0xFFFF:
raise TensorBoardServerException(
'TensorBoard cannot bind to port %d > %d' % (base_port, 0xFFFF)
)
max_attempts = 10 if should_scan else 1
base_port = min(base_port + max_attempts, 65536) - max_attempts
base_port = min(base_port + max_attempts, 0x10000) - max_attempts
Copy link
Contributor

@nfelt nfelt Feb 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading this logic, is this line really necessary? It can't actually be reached in the current code, it would only happen if we set the default base_port to a number within 10 of 65536.

Copy link
Contributor Author

@wchargin wchargin Feb 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s correct. I should add to the test plan that I did in fact test
this by changing the core_plugin.DEFAULT_PORT to 65534 and observing
that it binds to 65526 instead.

(Arguably it would be nicer to count down in such a case, but for code
that is never executed I have a limited budget for aesthetics.)


self._auto_wildcard = False
if not host:
# Without an explicit host, we default to serving on all interfaces,
# and will attempt to serve both IPv4 and IPv6 traffic through one socket.
host = self._get_wildcard_address(base_port)
self._auto_wildcard = True
# Without an explicit host, we default to serving on all interfaces,
# and will attempt to serve both IPv4 and IPv6 traffic through one
# socket.
self._auto_wildcard = not host

for (attempt_index, port) in (
enumerate(xrange(base_port, base_port + max_attempts))):
if self._auto_wildcard:
host = self._get_wildcard_address(port)
try:
# Yes, this invokes the super initializer potentially many
# times. This seems to work fine, and looking at the superclass
# chain (type(self).__mro__) it doesn't seem that anything
# _should_ go wrong (nor does any superclass provide a facility
# to do this natively).
#
# TODO(@wchargin): Refactor so that this is no longer necessary,
# probably by switching composing around a Werkzeug server
# rather than inheriting from it.
super(WerkzeugServer, self).__init__(host, port, wsgi_app)
break
except socket.error as e:
Expand Down