GODRIVER-3255 Await heartbeat checks upto freq when polling

[prestonvasquez]

Summary

Align the heartbeat check with driver specifications:

if (streamingEnabled and (serverSupportsStreaming or connectionIsStreaming)) or transitionedWithNetworkError:
    continue

Background & Motivation

When serverMonitoringMode=poll, SDAM awaits is not awaiting the full heartbeatFrequency before handshaking with the server.

[mongodb-drivers-pr-bot]

API Change Report

No changes found!

[matthewdale]

Looks good with one suggestion. 👍

[matthewdale]

Optional: This seems to rely on precise sleep synchronization and may intermittently fail if the scheduler responds to the timer goroutine slightly after the ticker goroutine (on the 4th "tick"). Consider using a more reliable assertion, like asserting a maximum number of heartbeats during an elapsed time.

E.g.

start := time.Now()
time.Sleep(heartbeatInterval * 4)
got := heartbeatStartedCount.Load()

elapsed := time.Since(start)
wantMax := elapsed / heartbeatInterval

assert.LessOrEqual(mt,
	got,
	wantMax,
	"expected fewer than %d heartbeats in %v",
	wantMax,
	elapsed)

It may also be helpful to further reduce the heartbeat interval to speed up the test.

[prestonvasquez]

Good point! A more robust solution would be to assert that the number of handshakes align with the number of discovered servers within minHeartbeatFrequencyMS.

[matthewdale]

Ah, right, you'd have to know how many servers connected 🤔 . I believe you could derive that from the heartbeat events by counting distinct addresses.

[prestonvasquez]

We could also make this a strong equality.

[matthewdale]

I think that could cause failures in some cases. Go timers (including tickers) guarantee that a goroutine will continue no earlier than the deadline set. However, there are no strong guarantees for how soon after the deadline the goroutine will continue. As a result, it's possible (though unlikely) that the polling loop will not continue as many times as expected during the test duration.

[prestonvasquez]

I'm fairly certain there are no concurrency concerns here since this is a set (i.e. nothing is aggregated).

[matthewdale]

Go maps are not concurrent safe except for read-only use, independent of the keys or data written. Here's the data race detector error:

WARNING: DATA RACE
Read at 0x00c000511470 by goroutine 122:
  go.mongodb.org/mongo-driver/mongo/integration.TestServerHeartbeatStartedEvent.func2()
      /mongo-go-driver/mongo/integration/sdam_prose_test.go:266 +0x370
  go.mongodb.org/mongo-driver/mongo/integration/mtest.(*T).Run.(*T).RunOpts.func1()
      /mongo-go-driver/mongo/integration/mtest/mongotest.go:267 +0x2ac
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1689 +0x180
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1742 +0x40

Previous write at 0x00c000511470 by goroutine 195:
  runtime.mapaccess2_faststr()
      /usr/local/go/src/runtime/map_faststr.go:108 +0x42c
  go.mongodb.org/mongo-driver/mongo/integration.TestServerHeartbeatStartedEvent.func2.2()
      /mongo-go-driver/mongo/integration/sdam_prose_test.go:249 +0xb8
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Topology).publishTopologyDescriptionChangedEvent()
      /mongo-go-driver/x/mongo/driver/topology/topology.go:1067 +0x224
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Topology).apply()
      /mongo-go-driver/x/mongo/driver/topology/topology.go:973 +0xe98
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.New.func1()
      /mongo-go-driver/x/mongo/driver/topology/topology.go:163 +0x78
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Server).updateDescription()
      /mongo-go-driver/x/mongo/driver/topology/server.go:707 +0x140
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Server).update.func3()
      /mongo-go-driver/x/mongo/driver/topology/server.go:624 +0xd8
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Server).update()
      /mongo-go-driver/x/mongo/driver/topology/server.go:654 +0x4f0
  go.mongodb.org/mongo-driver/x/mongo/driver/topology.(*Server).Connect.gowrap1()
      /mongo-go-driver/x/mongo/driver/topology/server.go:252 +0x34

[prestonvasquez]

For the sake of this test it seems like it shouldn't matter. We're just checking that the number of heartbeats doesn't get ahead of discovery, AFAIK the size of the set doesn't have to be precise at the time we make the assertion. Anyway, updated with a mutex.

[prestonvasquez]

Even though there is a race concern I don't think you can actually break what is being tested. I wasn't sure the race detector would care in this case 🤷

[qingyang-hu]

LGMT!