#819

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

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 v7_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, 534 of 535 specs pass and no data races are reported. The one failure (Truncate ... must truncate the whole namespace) also fails on v7_stage when 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.