Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GODRIVER-1778 Reject HeartbeatFrequencyMS of less than 500ms #1828

Merged
merged 10 commits into from
Oct 2, 2024
6 changes: 3 additions & 3 deletions internal/integration/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,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 @@ -516,7 +516,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 @@ -554,7 +554,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 @@ -89,7 +89,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 @@ -45,7 +45,7 @@ const (
)

var (
defaultHeartbeatInterval = 50 * time.Millisecond
defaultHeartbeatInterval = 500 * time.Millisecond
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the history of the original 50ms default? Does the 500ms requirement conflict with legacy test specifications? Or was this value chosen arbitrarily?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it came from this commit to implement streaming heartbeat protocol, not sure how the value was chosen though.

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) {
joyjwang marked this conversation as resolved.
Show resolved Hide resolved
testCases := []struct {
name string
opts *ClientOptionsBuilder
err error
}{
{
joyjwang marked this conversation as resolved.
Show resolved Hide resolved
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
Loading