Skip to content

Commit

Permalink
GODRIVER-1778 Reject HeartbeatFrequencyMS of less than 500ms (#1828)
Browse files Browse the repository at this point in the history
  • Loading branch information
joyjwang authored Oct 2, 2024
1 parent c42db6b commit e22ca8f
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 14 deletions.
6 changes: 3 additions & 3 deletions internal/integration/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ type slowConnDialer struct {
}

var slowConnDialerDelay = 300 * time.Millisecond
var reducedHeartbeatInterval = 100 * time.Millisecond
var reducedHeartbeatInterval = 500 * time.Millisecond

func newSlowConnDialer(delay time.Duration) *slowConnDialer {
return &slowConnDialer{
Expand Down Expand Up @@ -517,7 +517,7 @@ func TestClient(t *testing.T) {
mt.Parallel()

// Reset the client with a dialer that delays all network round trips by 300ms and set the
// heartbeat interval to 100ms to reduce the time it takes to collect RTT samples.
// heartbeat interval to 500ms to reduce the time it takes to collect RTT samples.
mt.ResetClient(options.Client().
SetDialer(newSlowConnDialer(slowConnDialerDelay)).
SetHeartbeatInterval(reducedHeartbeatInterval))
Expand Down Expand Up @@ -555,7 +555,7 @@ func TestClient(t *testing.T) {
assert.Nil(mt, err, "Ping error: %v", err)

// Reset the client with a dialer that delays all network round trips by 300ms and set the
// heartbeat interval to 100ms to reduce the time it takes to collect RTT samples.
// heartbeat interval to 500ms to reduce the time it takes to collect RTT samples.
tpm := eventtest.NewTestPoolMonitor()
mt.ResetClient(options.Client().
SetPoolMonitor(tpm.PoolMonitor).
Expand Down
2 changes: 1 addition & 1 deletion internal/integration/unified/unified_spec_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ var (
}

logMessageValidatorTimeout = 10 * time.Millisecond
lowHeartbeatFrequency = 50 * time.Millisecond
lowHeartbeatFrequency = 500 * time.Millisecond
)

// TestCase holds and runs a unified spec test case
Expand Down
2 changes: 1 addition & 1 deletion internal/integration/unified_spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const (
)

var (
defaultHeartbeatInterval = 50 * time.Millisecond
defaultHeartbeatInterval = 500 * time.Millisecond
skippedTestDescriptions = map[string]string{
// SPEC-1403: This test checks to see if the correct error is thrown when auto encrypting with a server < 4.2.
// Currently, the test will fail because a server < 4.2 wouldn't have mongocryptd, so Client construction
Expand Down
8 changes: 7 additions & 1 deletion mongo/options/clientoptions.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,11 @@ func (c *ClientOptionsBuilder) Validate() error {
}
}

if args.HeartbeatInterval != nil && *args.HeartbeatInterval < (500*time.Millisecond) {
return fmt.Errorf("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=%q",
*args.HeartbeatInterval)
}

if args.MaxPoolSize != nil && args.MinPoolSize != nil && *args.MaxPoolSize != 0 &&
*args.MinPoolSize > *args.MaxPoolSize {
return fmt.Errorf("minPoolSize must be less than or equal to maxPoolSize, got minPoolSize=%d maxPoolSize=%d",
Expand Down Expand Up @@ -755,7 +760,8 @@ func (c *ClientOptionsBuilder) SetDirect(b bool) *ClientOptionsBuilder {
}

// SetHeartbeatInterval specifies the amount of time to wait between periodic background server checks. This can also be
// set through the "heartbeatIntervalMS" URI option (e.g. "heartbeatIntervalMS=10000"). The default is 10 seconds.
// set through the "heartbeatFrequencyMS" URI option (e.g. "heartbeatFrequencyMS=10000"). The default is 10 seconds.
// The minimum is 500ms.
func (c *ClientOptionsBuilder) SetHeartbeatInterval(d time.Duration) *ClientOptionsBuilder {
c.Opts = append(c.Opts, func(opts *ClientOptions) error {
opts.HeartbeatInterval = &d
Expand Down
54 changes: 54 additions & 0 deletions mongo/options/clientoptions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,60 @@ func TestClientOptions(t *testing.T) {
})
}
})
t.Run("heartbeatFrequencyMS validation", func(t *testing.T) {
testCases := []struct {
name string
opts *ClientOptionsBuilder
err error
}{
{
name: "heartbeatFrequencyMS > minimum (500ms)",
opts: Client().SetHeartbeatInterval(10000 * time.Millisecond),
err: nil,
},
{
name: "heartbeatFrequencyMS == minimum (500ms)",
opts: Client().SetHeartbeatInterval(500 * time.Millisecond),
err: nil,
},
{
name: "heartbeatFrequencyMS < minimum (500ms)",
opts: Client().SetHeartbeatInterval(10 * time.Millisecond),
err: errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"10ms\""),
},
{
name: "heartbeatFrequencyMS == 0",
opts: Client().SetHeartbeatInterval(0 * time.Millisecond),
err: errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"0s\""),
},
{
name: "heartbeatFrequencyMS > minimum (500ms) from URI",
opts: Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=10000"),
err: nil,
},
{
name: "heartbeatFrequencyMS == minimum (500ms) from URI",
opts: Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=500"),
err: nil,
},
{
name: "heartbeatFrequencyMS < minimum (500ms) from URI",
opts: Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=10"),
err: errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"10ms\""),
},
{
name: "heartbeatFrequencyMS == 0 from URI",
opts: Client().ApplyURI("mongodb://localhost:27017/?heartbeatFrequencyMS=0"),
err: errors.New("heartbeatFrequencyMS must exceed the minimum heartbeat interval of 500ms, got heartbeatFrequencyMS=\"0s\""),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
err := tc.opts.Validate()
assert.Equal(t, tc.err, err)
})
}
})
t.Run("minPoolSize validation", func(t *testing.T) {
testCases := []struct {
name string
Expand Down
16 changes: 8 additions & 8 deletions x/mongo/driver/topology/polling_srv_records_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ func compareHosts(t *testing.T, received []description.Server, expected []string

func TestPollingSRVRecordsSpec(t *testing.T) {
for _, uri := range []string{
"mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100",
"mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500",
// Test with user:pass as a regression test for GODRIVER-2620
"mongodb+srv://user:[email protected]/?heartbeatFrequencyMS=100",
"mongodb+srv://user:[email protected]/?heartbeatFrequencyMS=500",
} {
t.Run(uri, func(t *testing.T) {
testPollingSRVRecordsSpec(t, uri)
Expand Down Expand Up @@ -169,7 +169,7 @@ func testPollingSRVRecordsSpec(t *testing.T, uri string) {

func TestPollSRVRecords(t *testing.T) {
t.Run("Not unknown or sharded topology", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -207,7 +207,7 @@ func TestPollSRVRecords(t *testing.T) {

})
t.Run("Failed Hostname Verification", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand All @@ -234,7 +234,7 @@ func TestPollSRVRecords(t *testing.T) {

})
t.Run("Return to polling time", func(t *testing.T) {
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -276,7 +276,7 @@ func TestPollingSRVRecordsLoadBalanced(t *testing.T) {
}

t.Run("pollingRequired is set to false", func(t *testing.T) {
topo := createLBTopology(t, "mongodb+srv://test24.test.build.10gen.cc/?heartbeatFrequencyMS=100")
topo := createLBTopology(t, "mongodb+srv://test24.test.build.10gen.cc/?heartbeatFrequencyMS=500")
assert.False(t, topo.pollingRequired, "expected SRV polling to not be required, but it is")
})

Expand Down Expand Up @@ -313,7 +313,7 @@ func TestPollSRVRecordsMaxHosts(t *testing.T) {
simulateSRVPoll := func(srvMaxHosts int, recordsToAdd []*net.SRV, recordsToRemove []*net.SRV) (*Topology, func(ctx context.Context) error) {
t.Helper()

uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=100"
uri := "mongodb+srv://test1.test.build.10gen.cc/?heartbeatFrequencyMS=500"
cfg, err := NewConfig(options.Client().ApplyURI(uri).SetSRVMaxHosts(srvMaxHosts), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down Expand Up @@ -385,7 +385,7 @@ func TestPollSRVRecordsServiceName(t *testing.T) {
simulateSRVPoll := func(srvServiceName string, recordsToAdd []*net.SRV, recordsToRemove []*net.SRV) (*Topology, func(ctx context.Context) error) {
t.Helper()

uri := "mongodb+srv://test22.test.build.10gen.cc/?heartbeatFrequencyMS=100&srvServiceName=customname"
uri := "mongodb+srv://test22.test.build.10gen.cc/?heartbeatFrequencyMS=500&srvServiceName=customname"
cfg, err := NewConfig(options.Client().ApplyURI(uri).SetSRVServiceName(srvServiceName), nil)
require.NoError(t, err, "error constructing topology config: %v", err)

Expand Down

0 comments on commit e22ca8f

Please sign in to comment.