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 · 6 comments

Comments

@sjlee
Copy link

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

@gopherbot gopherbot added the Proposal label Nov 2, 2017

@sjlee

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

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

@sjlee

This comment has been minimized.

Copy link
Author

commented Nov 2, 2017

Sure. FYI, I changed the title only.

@sjlee

This comment has been minimized.

Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.