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

x/net/http2: support graceful close of client connection #17292

Closed
rs opened this issue Sep 30, 2016 · 7 comments
Closed

x/net/http2: support graceful close of client connection #17292

rs opened this issue Sep 30, 2016 · 7 comments
Assignees
Labels
Milestone

Comments

@rs
Copy link
Contributor

@rs rs commented Sep 30, 2016

I'm implementing my own http2.ConnPool with some health checking done using the new Ping() method. My problem is that my pool doesn't have the capability to dispose client connections because there is no way to cleanly close a client connection other than keeping it's net.Conn on the side and abruptly close it.

What about adding a Close() method http2.ClientConn that would send a GOAWAY frame and wait for the in-flight streams to complete?

Implementation proposal: https://go-review.googlesource.com/#/c/30076/

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 30, 2016

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

@quentinmit quentinmit added this to the Unreleased milestone Oct 4, 2016
@quentinmit quentinmit added the NeedsFix label Oct 4, 2016
@rs

This comment has been minimized.

Copy link
Contributor Author

@rs rs commented Mar 16, 2017

This PR has been sitting for month now. Is there something to be done to resume the review?

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Mar 16, 2017

Sorry, that CL (its tests mostly) exhausted me earlier and then I forgot about it during Go 1.8 chaos.

Its tests still need a bit of clarity about what they're trying to do. The test code itself might be fine now, but it's hard to tell without comments saying what the intent is.

It looks like the non-test code is still fine.

/cc @tombergan

@rs

This comment has been minimized.

Copy link
Contributor Author

@rs rs commented Mar 16, 2017

I am sorry to hear tests exhausted you :)
I added some comments to clarify the tests.

@rs

This comment has been minimized.

Copy link
Contributor Author

@rs rs commented Jul 9, 2018

Thank you @bradfitz

@gaorong

This comment has been minimized.

Copy link

@gaorong gaorong commented Apr 24, 2019

I'm implementing my own http2.ConnPool with some health checking done using the new Ping() method.

@rs sorry to bother you, I want to find a proper way to use Ping() method, but can hardly find some valuable reference, would you mind sharing your implement of ConnPolol? thanks

@rs

This comment has been minimized.

Copy link
Contributor Author

@rs rs commented Apr 24, 2019

Our final implementation ended up not using ping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.