#818
fix: CLIENT-5774 close node connections when the initial cluster tend times out
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 returnsnil, TIMEOUT.waitTillStabilizedcalledCluster.Close(), butclusterBosshad not been started yet, and nodes were only closed at the end ofclusterBoss. The initial tend goroutine was not tracked bywgTendand kept tending after the timeout, and eachNode.Refreshfilled the pool up toMinConnectionsPerNode. 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 withTIMEOUT.clusterBossstarted tending while the initial tend goroutine was still running, sotend()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 afterClose()returned.
Changes
- The initial tend goroutine is tracked in
wgTendand stops once the cluster is closed. clusterBosswaits for the initial tend goroutine to exit before its first tend, sotend()never runs on two goroutines.Cluster.Close()closes the nodes after both tend goroutines have exited, instead of relying onclusterBossto do it. This covers the case whereclusterBosswas never started.- When
FailIfNotConnectedis true and cluster creation fails,NewClustercloses the cluster in a separate goroutine. The initial tend can be in the middle of info requests that each take up toClientPolicy.Timeout, so the constructor still returns atClientPolicy.Timeoutinstead 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 madeClose()hang. - Add
cluster_init_test.go. It connects through a local TCP proxy that delays server responses, so the initial tend outlastsClientPolicy.Timeoutand the connections opened through the proxy can be counted.
Testing
- The new
FailIfNotConnected: truespec fails onstage, 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 withFailIfNotConnected: falsethe client finishes connecting in the background. They pass on both branches. Reproducing a node being added afterClose()depends on timing, so no spec covers that case deterministically. - The new specs passed three runs in a row with
-raceagainst 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 namespaceand twoExpErrorDetailCDT specs) also fail onstagewhen run on their own, and are unrelated to this change.
v7 has the same pattern. The v7 backport is #819.
Activity
2mirzakaracicrequested changes2d 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.