#819
fix: CLIENT-5774 close node connections when the initial cluster tend times out (v7)
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 onv7_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 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, 534 of 535 specs pass and no data races are reported. The one failure (Truncate ... must truncate the whole namespace) also fails onv7_stagewhen run on its own, and is unrelated to this change. go build -tags as_proxy ./...passes.
This is the v7 backport of #818. The change is the same, except that v7 stores ClientPolicy by value on Cluster instead of in an atomic pointer.