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

net/http: Transport: add a ConnectionManager interface to separate the connection management from http.Transport #22537

Open
sjlee opened this issue Nov 2, 2017 · 10 comments
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@sjlee
Copy link

sjlee commented Nov 2, 2017

http.Transport gives us a real solid HTTP client functionality and a faithful protocol implementation. The connection pooling/management is also bundled into http.Transport. http.Transport connection management takes a stance on a few areas. For example,

  • it does not limit the number of active connections
  • it reuses available connections in a LIFO manner

There are real needs and use cases where we need a different behavior there. We may want to limit the number of active connections. We may want to have a different connection pooling policy (e.g. FIFO). But today it is not possible if you use http.Transport. The only option is to implement the HTTP client, but we like the protocol implementation that exists in http.Transport.

There are several issues filed because of the inability to override or modify the connection management behavior of http.Transport:

among others.

It would be great if the connection management aspect of http.Transport is separated from the protocol aspect of http.Transport and becomes pluggable (e.g. a ConnectionManager interface). Then we could choose to provide a different connection management implementation and mix it with the protocol support of http.Transport.

The http.Transport API would add this new optional field:

type Transport struct {
    ...
    // ConnMgr provides the connection management behavior
    // if nil, a default connection manager is used (yet to be named)
    ConnMgr ConnectionManager
}

The connection manager should have a fairly simple API while encapsulating the complex behavior in the implementation. An incomplete API might look like:

type ConnectionManager interface {
    Get() (net.Conn, error)
    Put(conn net.Conn)
}

It would be a pretty straightforward pool-like API. The only wrinkle might be that it should allow for the possibility that Get may be blocking (with a timeout) for certain implementations that want to allow timed waits for obtaining a connection from the connection manager.

It'd be great if the current "connection manager" is available publicly so some implementations can start with the base implementation and configure/customize it or override some methods as needed.

@gopherbot gopherbot added this to the Proposal milestone Nov 2, 2017
@sjlee
Copy link
Author

sjlee commented Nov 2, 2017

One interesting thing is that there are some existing fields in http.Transport that are part of the connection manager concerns: MaxIdleConns, MaxIdleConnsPerHost, and IdleConnTimeout. They probably would need to be deprecated as part of this proposal...

@tombergan
Copy link
Contributor

I like where this is going but it's not a full-fledged proposal yet. It is more of a feature request at this point. See the golang-dev thread for questions that would need to be answered in a full-fledged proposal:
https://groups.google.com/forum/#!topic/golang-dev/UTrl-mT1bhU

@sjlee
Copy link
Author

sjlee commented Nov 2, 2017

Noted. Let me reword it.

@sjlee sjlee changed the title proposal: net/http.Transport: add a ConnectionManager interface to separate the connection management from http.Transport net/http.Transport: add a ConnectionManager interface to separate the connection management from http.Transport Nov 2, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2017

Please append new comments instead of editing history to keep conversation easier to follow.

@sjlee
Copy link
Author

sjlee commented Nov 2, 2017

Sure. FYI, I changed the title only.

@sjlee
Copy link
Author

sjlee commented Nov 21, 2017

Wanted to rekindle this discussion. I took a quick look at the current Transport code (and the H2 counterpart), and had some thoughts and questions.

In principle, we could narrowly define the ClientConnPool (I now prefer this name over ConnManager as that is more inline with the H2 types) as a fairly simple map-like type. We could even exclude the dial aspect and leave that as part of Transport.

In the HTTP/1.x code, it basically boils down to isolating code that uses idleConn, idleConnCh, idleLRU, and MaxIdleConns and MaxIdleConnsPerHost and providing Get/Put methods on it. I see the following methods as methods that primarily interact with those fields:

  • getIdleConn()
  • tryPutIdleConn()
  • CloseIdleConnections()
  • removeIdleConnLocked()
  • getIdleConnCh()
  • closeConnIfStillIdle()

I could see (parts of) most of these methods forming the basis of the default ClientConnPool implementation. It appears that the aspects of managing idle connections are fairly well-isolated already in these methods so hopefully the refactoring shouldn't be terribly difficult.

I also see persistConn play some role in handling idle connections. I suspect one would want persistConn to be on the side of Transport, and have ClientConnPool produce and cache primarily at the level of net.Conn.

Regarding the proxy, @tombergan said:

The second challenge is handling proxies. Not only do we need to consider all types of proxies we support currently (see connectMethod in net/http/transport.go), but we also need to define the interface broadly enough that it is possible to support other kinds of proxies later (as one example, see the recently-added support for https proxies).

Maybe I don't fully recognize the subtlety here, but at the end of the day getting a proxy connection still seems to boil down to a key and looking up a connection based on that key:

func (t *Transport) getIdleConn(cm connectMethod) (pconn *persistConn, idleSince time.Time) {
	key := cm.key()
	t.idleMu.Lock()
	defer t.idleMu.Unlock()
	for {
		pconns, ok := t.idleConn[key]
  ...

All interactions with the idle connections seem to be driven by the opaque key. Are there more one needs to be concerned with?

@ianlancetaylor ianlancetaylor changed the title net/http.Transport: add a ConnectionManager interface to separate the connection management from http.Transport net/http: Transport: add a ConnectionManager interface to separate the connection management from http.Transport Aug 7, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Unplanned Aug 7, 2020
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 7, 2020
@swengineer404
Copy link

Any updates on this?

@suiriass
Copy link

Any updates on this? All interactions with the idle connections seem to be driven by the opaque key. In some cases,need get idle connection by specific addr

@elindsey
Copy link

elindsey commented Oct 1, 2022

We could even exclude the dial aspect and leave that as part of Transport.

This is likely to be limiting - the pool needs some way to create connections for cases where we want to prewarm or maintain a certain number of waiting connections at all times. If dialing is left in Transport and not the pool, the Transport will probably need to expose something similar to http2.Transport's NewClientConn()

In the HTTP/1.x code, it basically boils down to isolating code that uses idleConn, idleConnCh, idleLRU, and MaxIdleConns and MaxIdleConnsPerHost and providing Get/Put methods on it
... I suspect one would want persistConn to be on the side of Transport, and have ClientConnPool produce and cache primarily at the level of net.Conn.

I'm having trouble understanding if http2 is or is not in scope. The original comment references two http2 tasks, but an API that only exposes Get/Put methods at the net.Conn level isn't a good fit for http2.

An API designed for http2 should also be suitable for http1, but the other way around isn't necessarily true. From the perspective of a pool, http1 is similar to http2 if each connection has a one inflight transaction limit.

An http2 API will generally need Get to be at the transaction rather than connection level, and instead of Put will need some number of lifecycle callbacks (on complete, on error, etc. depending on what features you want to be able to implement in the pool). The current http2.ClientConnPool API is part of the way there - it has an appropriate Get that does a transaction reservation, but it lacks the lifecycle callbacks.

Instead of working from the http1 direction, one way to iterate on this design might be to:

  1. Define the functionality we want people to be able to implement in their custom pools
  2. Determine the minimum set of methods that need to be added to http2.ClientConnPool to support 1
  3. Verify that http1.Transport could also be refactored to use that same set of methods in 2

I think that will help land at the minimal API that covers both protocols and supports a variety of custom pool logic. There may be complexities around evolving the current API in that direction, but I think it's hard to reason about that until there's some agreement on what that direction even is.

@xkos
Copy link

xkos commented Mar 26, 2024

Any updates on this?
I found out that the issue has been around for a long time. Where can I keep track this topic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants