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: expose CloseIfIdle and ClientConnPoolIdleCloser #17775

Open
rs opened this Issue Nov 3, 2016 · 7 comments

Comments

Projects
None yet
6 participants
@rs
Copy link
Contributor

rs commented Nov 3, 2016

The http2 Transport exposes a CloseIdleConnections method that will call the method of the same name on the ClientConnPool if it implements the private clientConnPoolIdleCloser interface. The ClientConnPool implementation calls the private method closeIfIdle on each pooled ClientConn.

If one want to create a custom ClientConnPool, there is no way to make this custom pool support this feature without exposing CloseIfIdle method and ClientConnPoolIdleCloser interface.

@rakyll rakyll added this to the Unreleased milestone Nov 3, 2016

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Nov 3, 2016

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

@rs

This comment has been minimized.

Copy link
Contributor

rs commented Aug 15, 2017

@bradfitz: any interest for this PR?

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Aug 15, 2017

I am on leave for a couple months, so I'll let @tombergan handle this.

But I do not think it's a good idea to expose tons of innards of http2. That constrains us in the future.

I'd rather see this bug phrased in terms of the problem rather than the solution.

@rs

This comment has been minimized.

Copy link
Contributor

rs commented Aug 15, 2017

We are implementing our own ClientConnPool. One feature of the default pool is its ability to trigger the closing of idle connections. To do that, it exposes a private interface clientConnPoolIdleCloser consisting of the method closeIdleConnections. When this interface is implemented, the transport call this method if CloseIdleConnections is called. First problem, there is no way for a custom pool to replicate this behavior.

But we could have our pool handling idle connections by itself, without relying on this Transport. CloseIdleConnections (that's what we do). But then, we miss another method used by the clientConnPool.closeIdleConnections method which is closeIfIdle on ClientConn.

@tombergan

This comment has been minimized.

Copy link
Contributor

tombergan commented Aug 15, 2017

This seems entirely reasonable. http2.Transport.CloseIdleConnections() is a no-op when the transport uses a custom ClientConnPool. This is a definite wart. I don't see a better way to do this other than exporting ClientConn.CloseIfIdle.

sideshow added a commit to sideshow/http2 that referenced this issue Aug 29, 2017

Expose CloseIfIdle and ClientConnPoolIdleCloser
Exposes CloseIfIdle and ClientConnPoolIdleCloser so connection pools can
correctly close idle connections

Fixes golang/go#17775
@rs

This comment has been minimized.

Copy link
Contributor

rs commented Nov 9, 2017

What is missing to merge this PR?

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Jan 7, 2018

...What is missing to merge this PR?

@rs just a minor doc nit that @tombergan had suggested then a rebase and we can run the trybots and get some approvals. @bradfitz as you are back, please feel free to take a look at the CL https://go-review.googlesource.com/c/net/+/32326.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment