http2 internal pool poc

[c3mb0]

I'm opening this PR to spark some discussion. This code is WIP, it lacks tests and proper flow, however functional.

Here's a small summary of how the http/2 connection pooling works in Go, as far as I understand:

1. Code wants to get a new connection
2. Internals check if there are any shared and available connections in the pool. If there is one, it returns the said connection
3. If there are none, internals check if there's an in-flight dial already. If there is, it waits for the result then returns the opened connection
4. As the last resort, internals start a dial, wait for the result, add the resulting connection to the pool, then returns the connection

You can browse the code here. Instead of writing a custom connection pool, I'm exploiting reflection here to trigger the 4th step.

First, CloseIdleConnections can be called to initialize the transport's internal connection pool, as seen here.

The function that checks if a connection is available is here. The most practical field to change in order to mark a connection as unavailable is closed. After retrieving and locking appropriate mutexes on both pool and connection level, the closed flag for all connections is set to true.

Following our scenario, the next connection request reaches step 4 since there are no "available" connections (although they are alive and well). Internals create a new connection, and add it to the pool. Since the resulting connection is obtained manually via GetClientConn, the returned connection can be pinged periodically in a separate goroutine.

Once the deed is done, closed flags for all connections in the pool are restored. Since dropped connections are cleaned automatically from the pool, when pinging fails, the desired connection amount can be sustained via addNewConn.

Reflected fields seem stable, they haven't been changed in 2 years. Since it is not executing on a hot path, reflect performance is not much of a concern.

Thoughts? Is it worth fleshing out, or would you be against a reflection based solution?

[jtorvald]

I'd love to see a better connection pool with warm connections. Currently it takes sometimes more than a minute to connect to Apple. I tried a lot of different settings but no result.

[sideshow]

Thanks for the PR @c3mb0 ! :)
I really want to move this forward as it's now blocking the token based auth too.
What are your thoughts about us patching the http transport to make the methods we need public?
We already vendor the http2 transport, so this would allow us to move forward without the need to use reflection?

[c3mb0]

That's reasonable, I chose reflection over patching to demonstrate the idea and avoid http2 maintenance. However, considering this issue that I came across afterwards, having our own patched http2 to fit our use case seems like the way to go. I'll get to work ASAP, expect an update this week.

[sideshow]

Cool, ideally we will only have to maintain our own patched version until there is consensus that these fields should be made public in the http2 master branch. Hopefully they open the fields up reasonably soon. See related issues golang/go#17776 and golang/go#17775

In the mean time, i've cloned and uploaded the http2 library here https://github.com/sideshow/http2
I've made some changes to allow us to (hopefully) move forward with the connection pool;

1. A GetClientConnInfo method - GetClientConnInfo returns information about the last active time,
   current requests, pending requests and maximum possible requests for a connection which is useful for connection pool logic. see https://github.com/sideshow/http2/commit/58fff6faf90e7afa6d5d3a62fd9c1c9da0c4560e

2. A FirstActive field - Helpful for connection pools which wish to
   terminate a connection which has been active for a specific period of time. see https://github.com/sideshow/http2/commit/a0ca267cab1892a81223d153e0bf7de456ed4c94

3. Expose CloseIfIdle and ClientConnPoolIdleCloser - So connections can be closed by the pool, as reported by x/net/http2: expose CloseIfIdle and ClientConnPoolIdleCloser golang/go#17775

[c3mb0]

Neat! I'll be sending some PR's your way, would you like to move the discussion over to the other repository or continue from here? If you prefer the former, you can mark this one as closed.

[nordicdyno]

just discussing defer a potential issue

[nordicdyno]

I think mu and rf should be passed as parameters here to avoid issue like this: https://play.golang.org/p/KKaK0776AK

[sideshow]

@c3mb0 closing this off. Feel free to start a new PR here or in the other repo. Thanks