Skip to content

proxy: add Dial (with context) #38

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

Closed
wants to merge 1 commit into from

Conversation

dweomer
Copy link
Contributor

@dweomer dweomer commented Mar 23, 2019

The existing API does not allow client code to take advantage of Dialer implementations that implement DialContext receivers. This a familiar API, see net.Dialer.

Fixes golang/go#27874
Fixes golang/go#19354
Fixes golang/go#17759
Fixes golang/go#13455

@gopherbot
Copy link
Contributor

This PR (HEAD: f4a0879) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR (HEAD: d71cd4f) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@dweomer dweomer force-pushed the proxy-dial-funcs branch 2 times, most recently from bdf06bb to 3ac08a0 Compare March 24, 2019 00:02
@gopherbot
Copy link
Contributor

This PR (HEAD: 3ac08a0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 3:

Please note that I was aware of https://go-review.googlesource.com/c/net/+/37641 before submitting this PR/CR. With this in mind I chose to merely add the proxy.Dial, proxy.DialTimeout, and proxy.DialContext functions along with the proxy.ContextDialer single-method interface. The proxy.direct and proxy.PerHost types have been modified to also implement proxy.ContextDialer so as to curry the context as deeply into the stack as possible and therefore minimize goroutine leakage. Legacy implementations of proxy.Dialer, when invoked from DialTimeout or DialContext, will temporarily leak a goroutine for as long as the underlying dialer implementation takes to timeout. Without changing the public API this seemed to me the best that could be hoped for.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@dweomer dweomer force-pushed the proxy-dial-funcs branch from 3ac08a0 to ad1e7a3 Compare May 1, 2019 17:41
@gopherbot
Copy link
Contributor

This PR (HEAD: ad1e7a3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 4:

(6 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@dweomer dweomer force-pushed the proxy-dial-funcs branch from ad1e7a3 to 52034fa Compare May 1, 2019 19:23
@gopherbot
Copy link
Contributor

This PR (HEAD: 52034fa) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@dweomer dweomer force-pushed the proxy-dial-funcs branch from 52034fa to cc96931 Compare May 1, 2019 19:26
@gopherbot
Copy link
Contributor

This PR (HEAD: cc96931) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 7: Commit message was updated.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 7:

(6 comments)

Patch Set 4:

(6 comments)

Brad, thank you for the feedback. I have incorporated your suggestions.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@dweomer dweomer changed the title proxy: add Dial, DialTimeout, and DialContext proxy: add Dial (with context) May 1, 2019
@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 9:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 9: Run-TryBot+1

(4 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 9:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=ca2c88a5


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 9: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 10:

(2 comments)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 10:

See my other comments on dial.go and socks5.go?


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 10:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 10:

Patch Set 10:

See my other comments on dial.go and socks5.go?

Yes, I will make the argument ordering change but I have a reply re: your func ProxyDial comment


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@dweomer dweomer force-pushed the proxy-dial-funcs branch from cc96931 to 72945cd Compare May 1, 2019 22:31
@gopherbot
Copy link
Contributor

This PR (HEAD: 72945cd) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 11:

(1 comment)

adjusted argument order, as suggested, for dialContext()


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 11:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 11:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

The existing API does not allow client code to take advantage of Dialer
implementations that implement DialContext receivers. This a familiar
API, see net.Dialer.

Fixes golang/go#27874
Fixes golang/go#19354
Fixes golang/go#17759
Fixes golang/go#13455
@dweomer dweomer force-pushed the proxy-dial-funcs branch from 72945cd to b0a3727 Compare May 2, 2019 18:14
@gopherbot
Copy link
Contributor

This PR (HEAD: b0a3727) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/net/+/168921 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Brad Fitzpatrick:

Patch Set 12: Run-TryBot+1 Code-Review+2


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=9a1750e0


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Jacob Blain Christen:

Patch Set 12:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 12: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/168921.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request May 2, 2019
The existing API does not allow client code to take advantage of Dialer implementations that implement DialContext receivers. This a familiar API, see net.Dialer.

Fixes golang/go#27874
Fixes golang/go#19354
Fixes golang/go#17759
Fixes golang/go#13455

Change-Id: I0f247783d2037da28c9917db99adda51db1647bd
GitHub-Last-Rev: b0a3727
GitHub-Pull-Request: #38
Reviewed-on: https://go-review.googlesource.com/c/net/+/168921
Reviewed-by: Brad Fitzpatrick <[email protected]>
Run-TryBot: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/168921 has been merged.

@gopherbot gopherbot closed this May 2, 2019
@dweomer dweomer deleted the proxy-dial-funcs branch May 2, 2019 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants