#818

fix: CLIENT-5774 close node connections when the initial cluster tend times out

Openjuliannguyen4wants to merge intofrom

Summary

When the initial cluster tend took longer than ClientPolicy.Timeout, NewClientWithPolicyAndHost could leave connections to the cluster nodes open with no client left to close them.

  • With FailIfNotConnected: true (the default), the constructor returns nil, TIMEOUT. waitTillStabilized called Cluster.Close(), but clusterBoss had not been started yet, and nodes were only closed at the end of clusterBoss. The initial tend goroutine was not tracked by wgTend and kept tending after the timeout, and each Node.Refresh filled the pool up to MinConnectionsPerNode. Those connections stayed open until the process exited, and every retry of client creation added another set.
  • With FailIfNotConnected: false, the constructor returns a client together with TIMEOUT. clusterBoss started tending while the initial tend goroutine was still running, so tend() ran on two goroutines at once, even though it assumes a single tend goroutine. Close() also did not stop the initial tend goroutine, so it could open connections after Close() returned.

Changes

  • The initial tend goroutine is tracked in wgTend and stops once the cluster is closed.
  • clusterBoss waits for the initial tend goroutine to exit before its first tend, so tend() never runs on two goroutines.
  • Cluster.Close() closes the nodes after both tend goroutines have exited, instead of relying on clusterBoss to do it. This covers the case where clusterBoss was never started.
  • When FailIfNotConnected is true and cluster creation fails, NewCluster closes the cluster in a separate goroutine. The initial tend can be in the middle of info requests that each take up to ClientPolicy.Timeout, so the constructor still returns at ClientPolicy.Timeout instead of waiting for them.
  • The initial tend goroutine's final send on its result channel no longer blocks. Close() now waits for this goroutine, so a blocked send would have made Close() hang.
  • Add cluster_init_test.go. It connects through a local TCP proxy that delays server responses, so the initial tend outlasts ClientPolicy.Timeout and the connections opened through the proxy can be counted.

Testing

  • The new FailIfNotConnected: true spec fails on stage, where 5 connections (MinConnectionsPerNode) stay open after the constructor returns, and passes with this change.
  • The other two specs check that Close() after a timeout leaves no connections open, and that with FailIfNotConnected: false the client finishes connecting in the background. They pass on both branches. Reproducing a node being added after Close() depends on timing, so no spec covers that case deterministically.
  • The new specs passed three runs in a row with -race against a local server.
  • On the full integration suite with -race, 1443 of 1446 specs pass and no data races are reported. The 3 failures (Truncate ... must truncate the whole namespace and two ExpErrorDetail CDT specs) also fail on stage when run on their own, and are unrelated to this change.

v7 has the same pattern. The v7 backport is #819.

Activity

2
mirzakaracicrequested changes2d ago
mirzakaracic2d ago

@juliannguyen4 the changes on this PR and v7 equivalen do fix the leak; however the problem is Close() now waits for the whole startup tend. With `FailIfNotConnected: false`` and responses slow but under Timeout, Close() right after the timeout for me locally took 2.6s / 7.8s / 26s for timeout 1s / 3s / 10s, vs 0.8s / 2.4s / 8s on stage. At the 30s default that's over a minute, and it also hits clientFinalizer, which runs on Go's single finalizer goroutine.

I think the wait is only there to protect the partition map cleanup in Close(), and that cleanup isn't needed anymore. Since Connection.Close() nils ctn.node, no finalizer object is left in the cycle and GC frees it. I checked: 400 create/close cycles grow the heap ~150KB with or without it, vs ~31MB when the clients are kept alive.

So the simpler version is: Keep your initialTendDone gate and the go Close() in NewCluster, drop the startup goroutine from wgTend, have it close the nodes on exit if the cluster was closed (Node.Close() is idempotent), and drop the partition cleanup. Let me know and I can show you the diff.