Skip to content

Conversation

@wchargin
Copy link
Contributor

Summary:
These tests are currently broken on IPv6-only systems. While it’s valid
to open a socket that binds to ("localhost", 0) even if the system
only supports IPv6, it’s not valid to start a Werkzeug server with
host localhost, because Werkzeug detects localhost as an IPv4
address and will open an AF_INET socket, causing catastrophe.

This restores the test behavior prior to #2589, which IMHO isn’t great,
but unbreaks these tests on IPv6-only systems.

Test Plan:
This test still works on my dual-IPv4/IPv6 machine, and now also works
on an IPv6-only machine where previously it failed with EAFNOSUPPORT.

wchargin-branch: program-test-bind-all

Summary:
These tests are currently broken on IPv6-only systems. While it’s valid
to open a _socket_ that binds to `("localhost", 0)` even if the system
only supports IPv6, it’s not valid to start a _Werkzeug server_ with
host `localhost`, because Werkzeug detects `localhost` as an IPv4
address and will open an `AF_INET` socket, causing catastrophe.

This restores the test behavior prior to #2589, which IMHO isn’t great,
but unbreaks these tests on IPv6-only systems.

Test Plan:
This test still works on my dual-IPv4/IPv6 machine, and now also works
on an IPv6-only machine where previously it failed with `EAFNOSUPPORT`.

wchargin-branch: program-test-bind-all
@wchargin
Copy link
Contributor Author

WANT_LGTM=any

@psybuzz
Copy link
Contributor

psybuzz commented Sep 23, 2019

Using host=None by default LGTM

@psybuzz psybuzz self-requested a review September 23, 2019 17:42
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

LGTM on the text fix, but does this have any implications for actually non-test usage of TensorBoard on an IPv6-only machine? Does Werkzeug fail in that case?

If so we might at least want to open a bug for fixing that, since at least occasionally people may have IPv6-only machines in the wild.

@wchargin
Copy link
Contributor Author

Yes; as far as I can tell, the now-default options will fail on an
IPv6-only machine. I don’t have any local IPv6-only machines to confirm,
but I opened #2691 for tracking.

@wchargin wchargin merged commit 1ee9243 into master Sep 23, 2019
@wchargin wchargin deleted the wchargin-program-test-bind-all branch September 23, 2019 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants