A possible file descriptor leak in ClientManager?

[cissoid]

Please ensure that you are using the latest version of the master branch before filing an issue.

Also make sure to include logs (if applicable) to help reproduce the issue by setting the GODEBUG=http2debug=2 env var.

1. What version of Go are you using? go version
   1.9.2

2. What OS and processor architecture are you using? go env
   linux amd64

3. What did you do?
   Here's my code simulate the situation which reach the MaxAge:

cert, _ := certificate.FromPemFile("cert.pem", "")
manager := apns2.NewClientManager()
manager.MaxAge = time.Second * 5
for {
    resp, err := manager.Get(cert).Production().Push(&apns2.Notification{Topic: apnsTopic, DeviceToken: deviceToken, Payload: payload})
    fmt.Println(resp, err)
    time.Sleep(time.Second * 6)
}

4. What did you expect to see?
   ClientManager create a new client, and the underlying connections of old client should be closed by GC or call client.CloseIdleConnections().

5. What did you see instead?
   netstat and lsof shows that number of open connections keep increasing.

Maybe ClientManager should close idle connections on removing old client?
If do this so, what if another goroutine using the same client while we call client.CloseIdleConnections()?

[imhoffd]

@cissoid Odd. Once the Client is removed from the ClientManager, the reference to it should be lost and Go should GC it and close the connections automatically, shouldn't it? Is there anything in the code that doesn't exhibit that behavior?

Each client has its own Transport, which means calling CloseIdleConnections() on it won't affect other clients. I would be concerned when a client is "checked out" from the ClientManager and being used. CloseIdleConnections() wouldn't affect it, but it also wouldn't clean up after itself properly when the client is removed.

[cissoid]

@dwieeb

Once the Client is removed from the ClientManager, the reference to it should be lost and Go should GC it and close the connections automatically

I took a look at go source code. After connection created, there will start a new goroutine to run read loop, so there's should have another reference point to the connection, see https://github.com/golang/net/blob/master/http2/transport.go#L573.

The read loop will close connection only if keepalive on current connection is disabled, and exit loop only when read error, see https://github.com/golang/net/blob/master/http2/transport.go#L1458.

I'm not sure but this might be the reason.

[imhoffd]

@cissoid That behavior makes sense, I suppose. We could set DisableKeepAlives to true on the transport after a client is removed.

[cissoid]

@dwieeb That not works, DisableKeepAlives only checked when read loop starting, change it after connection created seems no effect.