Prevent leak of file descriptors with multiple clients

[orivej]

Each HTTP client keeps alive up to 2 connections, so the number of clients created in the lifetime of an application must be constant to prevent exhaustion of file descriptors. The options to fix this are:

• keep one global client (in this patch for HTTP)
• do not keep connections open (in this patch for UNIX)
• keep a global pool of clients or transports
• export and document a method to close connections that calls (*http.Transport).CloseIdleConnections()
• document that users can only instantiate a small number of clients during the lifetime of an application

[orivej]

Related to #1308.

[slackpad]

Hi @orivej - this looks like a good change, though we could get most of the benefit without disabling keepalives, since all API clients will share one underlying HTTP client by default. Wanted to see if you had some other rationale for including that, or if it was just an optimization. Thanks!

[orivej]

I disable kept alive connections only for the UNIX socket client, since it is created anew for each Consul client.

[orivej]

I reviewed the code and noticed that when CONSUL_HTTP_SSL_VERIFY environment variable is set to false, instantiating and using Consul client more than once will still leak file descriptors. This is a harder problem to solve, but disabling connection pooling in this case may be the best solution among the simple ones. I added another patch to the pull request accordingly.

A not so great but probably acceptable solution to add finalizers to new http.Transport's is being discussed at fsouza/go-dockerclient#434.

It seems to me that the proper way forward is to implement an http.Transport that shares one (or maybe, one per library) connection pool among its instances. Until it is done and becomes stable, disabling connection pooling is better than letting clients that are instantiated repeatedly to starve out of file descriptors.

[orivej]

Currently Travis tests fail with:

    2015/12/17 02:29:56 http: proxy error: net/http: request canceled
    2015/12/17 02:29:56 [DEBUG] http: Request PUT /v1/session/create (1.252325ms)
--- FAIL: TestLock_MonitorRetry (1.39s)
    lock_test.go:447: err: failed to create session: Unexpected response code: 500 ()

Although my local tests pass. Can Travis tests failure be related to this change?

[slackpad]

@orivej the Travis failures are definitely not related to your change - we've been having trouble with CI today so I've been running things locally as well. Looking at the UNIX socket code in the Go library I don't think there's actually any support for keepalives for those, so it's probably not necessary to change.

I took a crack at doing pooling of all three of the cases here, drafting off your idea for the default transport variable and the other spots you identified - #1517. What do you think?

[orivej]

Your patch looks good. (I assume that no one will need to create clients for unix sockets at an arbitrary number of different paths.)

Note that cleanhttp has DefaultTransport specifically because default transport is not the same as &http.Transport{}, and while we are at changing this peace of code, you might prefer to make defaultInsecureTransport more like the default transport.

Looking at the UNIX socket code in the Go library I don't think there's actually any support for keepalives for those, so it's probably not necessary to change.

DisableKeepAlives is a misnomer, it is about connection pooling rather than about keepalives, and unix connections are pooled like other connections in Transport.putIdleConn. I have seen exhaustion of file descriptors for unix sockets due to this pooling in go-dockerclient.

[slackpad]

@orivej thanks for the clarification on the keepalives. Updating the insecure transport is a good improvement so I went ahead and added that and merged the change.

[slackpad]

I had missed some of the conversation around go-cleanhttp in other issues and didn't appreciate that we can probably fix this up at that level with a finalizer, so reverted #1517 for now while we check into that. That would be a better solution and simplify the API client code. I kicked this back open as a reminder to re-add the change to the insecure socket code, which is independent of that change, and to make sure things get resolved upstream before closing this down again.

[jefferai]

@orivej See fsouza/go-dockerclient#434 (comment) -- can we find a good way to fix this in cleanhttp so that everyone automatically benefits?

[slackpad]

Closing this now that there #1526 open to clean up the loose ends.