Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
network: Added reusable FastestHostPort() helper #6426
Conversation
frobware
suggested changes
Oct 11, 2016
Are the tests supposed to mock entries like bad.example.com, et al?
| +// successfully connected to - the one with the lowest latency - or an error if | ||
| +// none of the given hostPorts can be reached. | ||
| +// | ||
| +// Timeout should be short, e.g. between 100ms and 1000ms. |
frobware
Oct 11, 2016
Contributor
From the PR I guess nothing is using this but I wonder if the exemplary timeout value of 1s is too short on a heavily loaded system.
| + conn, _ := result.(net.Conn) // cannot fail | ||
| + defer conn.Close() | ||
| + | ||
| + fastest := conn.RemoteAddr().String() |
frobware
Oct 11, 2016
•
Edited 1 time
-
frobware
Oct 11, 2016
Contributor
-
Oct 11, 2016
Just return ParseHostPort(conn.RemoteAddr().String()), the variable assignment is just noise. The function name (should) capture the intent.
| +func (*HostPortSuite) TestFastestHostPortAllUnreachable(c *gc.C) { | ||
| + // All of the endpoints below should either block indefinitely (without a | ||
| + // timeout) or fail immediately. | ||
| + unreachableHPs := network.NewHostPorts(49151, // IANA reserved port (unreachable) |
frobware
Oct 11, 2016
•
Edited 1 time
-
frobware
Oct 11, 2016
Contributor
-
Oct 11, 2016
Is 49151 unreachable on windows? ;)
| + // timeout) or fail immediately. | ||
| + unreachableHPs := network.NewHostPorts(49151, // IANA reserved port (unreachable) | ||
| + "255.1.2.3", // IPv4 route unreachable | ||
| + "bad.example.com", // unresolvable |
frobware
Oct 11, 2016
Contributor
On my system:
$ dig bad.example.com +short
92.242.132.15
Are you expecting this to not resolve?
dimitern
Oct 11, 2016
•
Edited 1 time
-
dimitern
Oct 11, 2016
Contributor
-
Oct 11, 2016
frobware
Oct 11, 2016
•
Edited 1 time
-
frobware
Oct 11, 2016
Contributor
-
Oct 11, 2016
$ dig "bad host name" +short
92.242.132.15
As discussed I think we should only consider localhost in the unit tests.
| @@ -247,7 +247,7 @@ func EnsureFirstHostPort(first HostPort, hps []HostPort) []HostPort { | ||
| // successfully connected to - the one with the lowest latency - or an error if | ||
| // none of the given hostPorts can be reached. | ||
| // | ||
| -// Timeout should be short, e.g. between 100ms and 1000ms. | ||
| +// Timeout should be short, e.g. between 100ms and 3s. | ||
| func FastestHostPort(timeout time.Duration, hostPorts ...HostPort) (*HostPort, error) { |
| + endpoints := set.NewStrings(HostPortsToStrings(hostPorts)...) | ||
| + for _, endpoint := range endpoints.Values() { | ||
| + try.Start(func(stop <-chan struct{}) (io.Closer, error) { | ||
| + return net.DialTimeout("tcp", endpoint, timeout) |
frobware
Oct 11, 2016
Contributor
If this successfully connects (and others), when are they closed? The way I read the function is that we return the fastest port to accept but, once done, we don't actually need or want the connection.
|
Closing this - will re-propose against develop after the changes I made. |
dimitern
closed this
Oct 14, 2016
dimitern
referenced this pull request
Oct 14, 2016
Merged
network: Added ReachableHostPort() and a few helpers #6454
dimitern
deleted the
dimitern:lp-1616098-ssh-all-ips
branch
Oct 18, 2016
added a commit
that referenced
this pull request
Oct 18, 2016
dimitern commentedOct 11, 2016
Takes a timeout and a slice of []network.HostPort entires.
Uses parallel.Try internally to connect to all of them
concurrently, with the given timeout. Needed to overcome
the limitation of the openssh client hanging when trying
to connect to an unreachable or blackhole routes.
Prerequisite to fix http://pad.lv/1616098.