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

Jump to bottom

net: dialTCP should support cancelation #11225

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

net: dialTCP should support cancelation #11225

pmarks-net opened this issue Jun 15, 2015 · 11 comments
Labels
FrozenDueToAge
Milestone
Go1.6

Comments

@pmarks-net
Copy link
Contributor

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
Copy link

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.
85a5fce 
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
Copy link
Contributor

mikioh commented Jul 28, 2015

Also please follow #11626.

@gopherbot
Copy link

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

@pmarks-net
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor

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

@pmarks-net
Copy link
Contributor Author

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
Copy link
Contributor

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

@gopherbot
Copy link

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

mikioh pushed a commit that referenced this issue Dec 16, 2015
@cixtor
net: retighten test harnesses for dial cancelation
1dc2e7b 
Updates #11225.

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

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

gopherbot pushed a commit that referenced this issue Feb 23, 2016
@bradfitz
net: use dialTCP cancelation for DualStack dialing.
676550d 
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
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 10, 2016
@bradfitz
net: slowDialTCP should wait forever if no deadline exists.
92dfc12 
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
@0intro
net: enable TestDialParallel, TestDialerFallbackDelay and TestDialCan…
4331ab9 
…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
@0intro
net: enable DualStack mode on Plan 9
f947429 
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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.6
Development

No branches or pull requests
5 participants
@bradfitz @mikioh @ianlancetaylor @pmarks-net @gopherbot