Skip to content
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

net: dialTCP should support cancelation #11225

Closed
pmarks-net opened this issue Jun 15, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@pmarks-net
Copy link
Contributor

commented Jun 15, 2015

Brad suggested that I file this bug during the Happy Eyeballs code review.

Currently, the dialTCP function will block until the operation succeeds, fails, or times out. If a dialer decides that the connection is no longer useful, it must be leaked and left to die on its own time.

Instead, there should be some cancelation mechanism (e.g. a closeable chan struct{}) which causes dialTCP to clean up its socket and return errCanceled immediately. This might also be exposed via the Dialer interface, and for non-TCP protocols, so regular users can take advantage of it.

@pmarks-net pmarks-net changed the title dialTCP should support cancellation dialTCP should support cancelation Jun 15, 2015

@ianlancetaylor ianlancetaylor changed the title dialTCP should support cancelation net: dialTCP should support cancelation Jun 15, 2015

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jun 15, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jul 27, 2015

CL https://golang.org/cl/12712 mentions this issue.

gopherbot pushed a commit that referenced this issue Jul 28, 2015

net: Set finalDeadline from TestDialParallel to avoid leaked sockets.
I've also changed TestDialSerialAsyncSpuriousConnection for consistency,
although it always computes a finalDeadline of zero.

Note that #11225 is the root cause of the socket leak; this just hides
it from the unit test by restoring the shorter timeout.

Fixes #11878

Change-Id: Ie0037dd3bce6cc81d196765375489f8c61be74c2
Reviewed-on: https://go-review.googlesource.com/12712
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Paul Marks <pmarks@google.com>
@mikioh

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2015

Also please follow #11626.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 14, 2015

CL https://golang.org/cl/17821 mentions this issue.

@pmarks-net

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2015

With the pending fix, is it expected that dialTCP will return quickly when canceled, on every platform that Go supports?

If so, then next we can rewrite dialParallel to always wait for dialTCP, and not leak.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 14, 2015

@pmarks-net, I didn't do Windows or Plan 9 yet. I wanted to see what people thought about it first before I did more.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 14, 2015

(but yes, cleaning up dialParallel was a big part of the motivation)

@bradfitz bradfitz closed this in 24a83d3 Dec 15, 2015

@pmarks-net

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2015

I don't think this can be closed until tcpsock_plan9 is also cancelable, unless it's acceptable to force DualStack: false on plan9.

Making dialParallel not leak implies waiting for dialTCP, which is only feasible if dialTCP is guaranteed to return quickly.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

Plan 9 doesn't really matter. You can force DualStack to false there. Don't let Plan 9 hold anything back.

@gopherbot

This comment has been minimized.

Copy link

commented Dec 16, 2015

CL https://golang.org/cl/17880 mentions this issue.

mikioh pushed a commit that referenced this issue Dec 16, 2015

Mikio Hara
net: retighten test harnesses for dial cancelation
Updates #11225.

Change-Id: I6c33d577f144643781f370ba2ab0997d1c1a3820
Reviewed-on: https://go-review.googlesource.com/17880
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Feb 18, 2016

CL https://golang.org/cl/19390 mentions this issue.

gopherbot pushed a commit that referenced this issue Feb 23, 2016

net: use dialTCP cancelation for DualStack dialing.
The previous Happy Eyeballs implementation would intentionally leak
connections, because dialTCP could not be reliably terminated upon
losing the race.

Now that dialTCP supports cancelation (plan9 excluded), dialParallel can
wait for responses from both the primary and fallback racers, strictly
before returning control to the caller.

In dial_test.go, we no longer need Sleep to avoid leaks.
Also, fix a typo in the Benchmark IPv4 address.

Updates #11225
Fixes #14279

Change-Id: Ibf3fe5c7ac2f7a438c1ab2cdb57032beb8bc27b5
Reviewed-on: https://go-review.googlesource.com/19390
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Copy link

commented Mar 9, 2016

CL https://golang.org/cl/20493 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 10, 2016

net: slowDialTCP should wait forever if no deadline exists.
This allows TestDialerFallbackDelay to pass again on machines where IPv6
connections to nowhere fail quickly instead of hanging.

This bug appeared last month, when I deleted the slowTimeout constant.

Updates #11225
Fixes #14731

Change-Id: I840011eee571aab1041022411541736111c7fad5
Reviewed-on: https://go-review.googlesource.com/20493
Run-TryBot: Paul Marks <pmarks@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Mikio Hara <mikioh.mikioh@gmail.com>

gopherbot pushed a commit that referenced this issue Apr 19, 2016

net: enable TestDialParallel, TestDialerFallbackDelay and TestDialCan…
…cel on Plan 9

TestDialParallel, TestDialerFallbackDelay and TestDialCancel
require dialTCP to support cancellation, which has been
implemented for Plan 9 in CL 22144.

Updates #11225.
Updates #11932.

Change-Id: I3b30a645ef79227dfa519cde8d46c67b72f2485c
Reviewed-on: https://go-review.googlesource.com/22203
Run-TryBot: David du Colombier <0intro@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit that referenced this issue Apr 19, 2016

net: enable DualStack mode on Plan 9
DualStack mode requires dialTCP to support cancellation,
which has been implemented for Plan 9 in CL 22144.

Updates #11225.
Updates #11932.

Change-Id: I6e468363dc147326b097b604c122d5af80362787
Reviewed-on: https://go-review.googlesource.com/22204
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

@golang golang locked and limited conversation to collaborators Mar 13, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.