Goroutine leak in Instance.SetPeers

[bohde]

I'm currently testing a Kubernetes deployment of gubernator, and noticed the number of goroutines are growing linearly over time. Here's the graph we're seeing from our monitoring system.

This also results in a similar leak in memory. In order to diagnose this, I've deployed a version of gubernator with pprof endpoints enabled, and found that goroutines grow in 3 functions:

1. PeerClient.run
2. Interval.run
3. grpc.Dial

The root cause seems to be that in Instance.SetPeer, a new PeerClient is created for every PeerInfo without reusing any existing PeerClients. This causes a goroutine leak linearly proportional to the number of peers. In addition, there is no shutdown code for removed peers, so this code should leak a goroutine for every peer that is removed.

I suspect that this was exposed by some weird interaction with the Kubernetes integration and our test environment, since I see peer update logs every few minutes.

I have a fork where I'm testing the following changes that I would be happy to send a pull request for:

1. Add a Shutdown method to PeerClient that will close the request queue
2. Change PeerClient.run to send any enqueued requests when the request queue is closed
3. Change PeerClient.run to call interval.Stop on return
4. Reuse existing PeerClients inside Instance.SetPeers
5. Shutdown any removed PeerClients inside Instance.SetPeers

[thrawn01]

This would be great! As you mentioned, we are not seeing this in our prod environments because our peer sets are pretty stable. The hard part of this would be knowing when all in flight requests to a peer have completed. It's possible that we could just assume the peer is no longer fielding requests because it's been removed from the peer set. Thoughts?

[bohde]

It appears the there's an experimental API to wait for a given GRPC connection to enter a state, which could be used to build graceful shutdowns (https://godoc.org/google.golang.org/grpc#ClientConn.WaitForStateChange). A shutdown process that accounts for this may be as follows:

1. Close the request queue
2. Send the pending batch
3. Wait for connection state to be idle
4. Close the connection

I'll investigate whether this approach is viable.

[bohde]

I couldn't reliably build graceful shutdowns of the connection using the WaitForStateChange API, so ended up using a sync.WaitGroup to track in-flight requests instead.

The changes are in my PR #28.