diff --git a/.evergreen/config.yml b/.evergreen/config.yml index 7e3b30bc47..1062acca71 100644 --- a/.evergreen/config.yml +++ b/.evergreen/config.yml @@ -1045,6 +1045,7 @@ tasks: - name: sa-fmt tags: ["static-analysis"] commands: + - func: install-linters - func: run-make vars: targets: check-fmt diff --git a/Makefile b/Makefile index 6d2b8be9e3..3806e0acf9 100644 --- a/Makefile +++ b/Makefile @@ -47,7 +47,7 @@ build-tests: .PHONY: check-fmt check-fmt: - etc/check_fmt.sh $(PKGS) + etc/check_fmt.sh # check-modules runs "go mod tidy" then "go mod vendor" and exits with a non-zero exit code if there # are any module or vendored modules changes. The intent is to confirm two properties: @@ -69,7 +69,7 @@ doc: .PHONY: fmt fmt: - gofmt -l -s -w $(PKGS) + go fmt ./... .PHONY: lint lint: diff --git a/etc/check_fmt.sh b/etc/check_fmt.sh index 5485624ab5..9b7f77dfa6 100755 --- a/etc/check_fmt.sh +++ b/etc/check_fmt.sh @@ -1,11 +1,11 @@ #!/usr/bin/env bash -# check_fmt gopackages... -# Runs gofmt on given packages and checks that *_example_test.go files have wrapped lines. +# check_fmt +# Runs go fmt on all packages in the repo and checks that *_example_test.go files have wrapped lines. -gofmt_out="$(gofmt -l -s "$@")" +gofmt_out="$(go fmt ./...)" if [[ $gofmt_out ]]; then - echo "gofmt check failed for:"; + echo "go fmt check failed for:"; sed -e 's/^/ - /' <<< "$gofmt_out"; exit 1; fi @@ -16,7 +16,7 @@ fi # E.g ignored lines: # // "mongodb://ldap-user:ldap-pwd@localhost:27017/?authMechanism=PLAIN" # // (https://www.mongodb.com/docs/manual/core/authentication-mechanisms-enterprise/#security-auth-ldap). -lll_out="$(find "$@" -type f -name "*_examples_test.go" | lll -w 4 -l 80 -e '^\s*\/\/.+:\/\/' --files)" +lll_out="$(find . -type f -name "*_examples_test.go" | lll -w 4 -l 80 -e '^\s*\/\/.+:\/\/' --files)" if [[ $lll_out ]]; then echo "lll check failed for:"; diff --git a/mongo/crud_examples_test.go b/mongo/crud_examples_test.go index 8c70569d6e..554aa11a84 100644 --- a/mongo/crud_examples_test.go +++ b/mongo/crud_examples_test.go @@ -749,8 +749,9 @@ func ExampleClient_StartSession_withTransaction() { result, err := sess.WithTransaction( context.TODO(), func(sessCtx mongo.SessionContext) (interface{}, error) { - // Use sessCtx as the Context parameter for InsertOne and FindOne so - // both operations are run in a transaction. + // Use the mongo.SessionContext as the Context parameter for + // InsertOne and FindOne so both operations are run in the same + // transaction. coll := client.Database("db").Collection("coll") res, err := coll.InsertOne(sessCtx, bson.D{{"x", 1}}) diff --git a/mongo/integration/initial_dns_seedlist_discovery_test.go b/mongo/integration/initial_dns_seedlist_discovery_test.go index 10cd49bd0a..3e4ef08d5d 100644 --- a/mongo/integration/initial_dns_seedlist_discovery_test.go +++ b/mongo/integration/initial_dns_seedlist_discovery_test.go @@ -15,12 +15,15 @@ import ( "runtime" "strings" "testing" + "time" "go.mongodb.org/mongo-driver/bson" "go.mongodb.org/mongo-driver/internal/testutil/assert" + "go.mongodb.org/mongo-driver/mongo" "go.mongodb.org/mongo-driver/mongo/description" "go.mongodb.org/mongo-driver/mongo/integration/mtest" "go.mongodb.org/mongo-driver/mongo/options" + "go.mongodb.org/mongo-driver/mongo/readpref" "go.mongodb.org/mongo-driver/x/mongo/driver/connstring" "go.mongodb.org/mongo-driver/x/mongo/driver/topology" ) @@ -37,6 +40,7 @@ type seedlistTest struct { NumHosts *int `bson:"numHosts"` Error bool `bson:"error"` Options bson.Raw `bson:"options"` + Ping *bool `bson:"ping"` } func TestInitialDNSSeedlistDiscoverySpec(t *testing.T) { @@ -44,12 +48,18 @@ func TestInitialDNSSeedlistDiscoverySpec(t *testing.T) { defer mt.Close() mt.RunOpts("replica set", mtest.NewOptions().Topologies(mtest.ReplicaSet).CreateClient(false), func(mt *mtest.T) { + mt.Parallel() + runSeedlistDiscoveryDirectory(mt, "replica-set") }) mt.RunOpts("sharded", mtest.NewOptions().Topologies(mtest.Sharded).CreateClient(false), func(mt *mtest.T) { + mt.Parallel() + runSeedlistDiscoveryDirectory(mt, "sharded") }) mt.RunOpts("load balanced", mtest.NewOptions().Topologies(mtest.LoadBalanced).CreateClient(false), func(mt *mtest.T) { + mt.Parallel() + runSeedlistDiscoveryDirectory(mt, "load-balanced") }) } @@ -63,6 +73,24 @@ func runSeedlistDiscoveryDirectory(mt *mtest.T, subdirectory string) { } } +// runSeedlistDiscoveryPingTest will create a new connection using the test URI and attempt to "ping" the server. +func runSeedlistDiscoveryPingTest(mt *mtest.T, clientOpts *options.ClientOptions) { + ctx := context.Background() + + client, err := mongo.Connect(ctx, clientOpts) + assert.Nil(mt, err, "Connect error: %v", err) + + defer func() { _ = client.Disconnect(ctx) }() + + // Create a context with a timeout to prevent the ping operation from blocking indefinitely. + pingCtx, cancel := context.WithTimeout(ctx, 1*time.Second) + defer cancel() + + // Ping the server. + err = client.Ping(pingCtx, readpref.Nearest()) + assert.Nil(mt, err, "Ping error: %v", err) +} + func runSeedlistDiscoveryTest(mt *mtest.T, file string) { content, err := ioutil.ReadFile(file) assert.Nil(mt, err, "ReadFile error for %v: %v", file, err) @@ -131,6 +159,10 @@ func runSeedlistDiscoveryTest(mt *mtest.T, file string) { _, err := getServerByAddress(host, topo) assert.Nil(mt, err, "error finding host %q: %v", host, err) } + + if ping := test.Ping; ping == nil || *ping { + runSeedlistDiscoveryPingTest(mt, opts) + } } func buildSet(list []string) map[string]struct{} { @@ -230,6 +262,14 @@ func getServerByAddress(address string, topo *topology.Topology) (description.Se if err != nil { return description.Server{}, err } + + // If the selected server is a topology.SelectedServer, then we can get the description without creating a + // connect pool. + topologySelectedServer, ok := selectedServer.(*topology.SelectedServer) + if ok { + return topologySelectedServer.Description().Server, nil + } + selectedServerConnection, err := selectedServer.Connection(context.Background()) if err != nil { return description.Server{}, err diff --git a/mongo/integration/retryable_writes_prose_test.go b/mongo/integration/retryable_writes_prose_test.go index 11198c0ad3..1f496ea328 100644 --- a/mongo/integration/retryable_writes_prose_test.go +++ b/mongo/integration/retryable_writes_prose_test.go @@ -282,7 +282,7 @@ func TestRetryableWritesProse(t *testing.T) { require.True(mt, secondFailPointConfigured) - // Assert that the "NotWritablePrimary" error is returned. + // Assert that the "ShutdownInProgress" error is returned. require.True(mt, err.(mongo.WriteException).HasErrorCode(int(shutdownInProgressErrorCode))) }) } diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.json b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.json index 3f500acdc6..8e459115c1 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.json +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.json @@ -10,5 +10,6 @@ "loadBalanced": true, "ssl": true, "directConnection": false - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.yml b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.yml index c8395f7786..7a77944a30 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.yml +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-directConnection.yml @@ -11,3 +11,4 @@ options: loadBalanced: true ssl: true directConnection: false +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.json b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.json index f9719e760d..39bff5a23b 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.json +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.json @@ -9,5 +9,6 @@ "options": { "loadBalanced": true, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.yml b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.yml index a172bda52b..c373192f01 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.yml +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/loadBalanced-true-txt.yml @@ -8,3 +8,4 @@ hosts: options: loadBalanced: true ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.json b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.json index a18360ea64..474a314fd7 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.json +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.json @@ -10,5 +10,6 @@ "loadBalanced": true, "srvMaxHosts": 0, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.yml b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.yml index 986164e3b9..f223a8d558 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.yml +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero-txt.yml @@ -8,3 +8,4 @@ options: loadBalanced: true srvMaxHosts: 0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.json b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.json index bd85418117..dfc90dc96d 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.json +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.json @@ -10,5 +10,6 @@ "loadBalanced": true, "srvMaxHosts": 0, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.yml b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.yml index 7dc1593484..f8343e0e3e 100644 --- a/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.yml +++ b/testdata/initial-dns-seedlist-discovery/load-balanced/srvMaxHosts-zero.yml @@ -8,3 +8,4 @@ options: loadBalanced: true srvMaxHosts: 0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.json b/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.json index 1d57bdcb3c..3f14ff94e7 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.json @@ -11,5 +11,6 @@ "options": { "ssl": true, "directConnection": false - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.yml b/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.yml index 20a32da594..21de8dc771 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/direct-connection-false.yml @@ -8,3 +8,4 @@ hosts: options: ssl: true directConnection: false +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.json b/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.json new file mode 100644 index 0000000000..4493628be9 --- /dev/null +++ b/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.json @@ -0,0 +1,22 @@ +{ + "uri": "mongodb+srv://b*b%40f3tt%3D:%244to%40L8%3DMC@test3.test.build.10gen.cc/mydb%3F?replicaSet=repl0", + "seeds": [ + "localhost.test.build.10gen.cc:27017" + ], + "hosts": [ + "localhost:27017", + "localhost:27018", + "localhost:27019" + ], + "options": { + "replicaSet": "repl0", + "ssl": true + }, + "parsed_options": { + "user": "b*b@f3tt=", + "password": "$4to@L8=MC", + "db": "mydb?" + }, + "ping": false, + "comment": "Encoded user, pass, and DB parse correctly" +} diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.yml b/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.yml new file mode 100644 index 0000000000..4b25a46db5 --- /dev/null +++ b/testdata/initial-dns-seedlist-discovery/replica-set/encoded-userinfo-and-db.yml @@ -0,0 +1,19 @@ +uri: "mongodb+srv://b*b%40f3tt%3D:%244to%40L8%3DMC@test3.test.build.10gen.cc/mydb%3F?replicaSet=repl0" +seeds: + - localhost.test.build.10gen.cc:27017 +hosts: + - localhost:27017 + - localhost:27018 + - localhost:27019 +options: + replicaSet: repl0 + ssl: true +parsed_options: + user: "b*b@f3tt=" + password: "$4to@L8=MC" + db: "mydb?" +# Don't run a ping for URIs that include userinfo. Ping doesn't require authentication, so missing +# userinfo isn't a problem, but some drivers will fail handshake on a connection if userinfo is +# provided but incorrect. +ping: false +comment: Encoded user, pass, and DB parse correctly diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.json b/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.json index fd2e565c7b..682d32a742 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.json @@ -11,5 +11,6 @@ "options": { "loadBalanced": false, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.yml b/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.yml index 424d192072..c7e31da131 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/loadBalanced-false-txt.yml @@ -8,3 +8,4 @@ hosts: options: loadBalanced: false ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.json b/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.json index 9a8267eaeb..ebe3fe1e77 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.json @@ -12,5 +12,6 @@ "replicaSet": "repl0", "ssl": true }, + "ping": true, "comment": "Is correct, as returned host name shared the URI root \"test.build.10gen.cc\"." } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.yml b/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.yml index e77c4570d3..92b54ae57f 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/longer-parent-in-return.yml @@ -8,4 +8,5 @@ hosts: options: replicaSet: repl0 ssl: true +ping: true comment: Is correct, as returned host name shared the URI root "test.build.10gen.cc". diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.json b/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.json index cebb3b1ec3..9f7733de80 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.json @@ -11,5 +11,6 @@ "options": { "replicaSet": "repl0", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.yml b/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.yml index 395bcdc968..748d4634ea 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-result-default-port.yml @@ -8,3 +8,4 @@ hosts: options: replicaSet: repl0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.json b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.json index 622668c351..1d740b1b59 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.json @@ -11,5 +11,6 @@ "options": { "replicaSet": "repl0", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.yml b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.yml index 90a702cdbe..2f353ffb74 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record-multiple-strings.yml @@ -8,3 +8,4 @@ hosts: options: replicaSet: repl0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.json b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.json index 2385021ad4..ecdb0a7e2a 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.json @@ -12,5 +12,6 @@ "replicaSet": "repl0", "authSource": "thisDB", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.yml b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.yml index 9356eaa2c2..3bc0f0405c 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/one-txt-record.yml @@ -9,3 +9,4 @@ options: replicaSet: repl0 authSource: thisDB ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.json b/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.json index ec36cdbb00..e320c2ca3e 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.json @@ -12,5 +12,6 @@ "options": { "ssl": true, "srvServiceName": "customname" - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.yml index b6f25d0ca7..3bf9c2c676 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srv-service-name.yml @@ -9,3 +9,4 @@ hosts: options: ssl: true srvServiceName: "customname" +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.json b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.json index d9765ac663..70edacfd06 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.json @@ -13,5 +13,6 @@ "options": { "srvMaxHosts": 2, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.yml index 6d905809b9..b483279614 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-equal_to_srv_records.yml @@ -14,3 +14,4 @@ hosts: options: srvMaxHosts: 2 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.json b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.json index 494bb87687..72540ed408 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.json @@ -12,5 +12,6 @@ "options": { "srvMaxHosts": 3, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.yml index 03307f8110..363dd4fd8b 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-greater_than_srv_records.yml @@ -13,3 +13,4 @@ hosts: options: srvMaxHosts: 3 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.json b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.json index 66a5e90dad..a9d6dd6fd9 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.json @@ -9,5 +9,6 @@ "options": { "srvMaxHosts": 1, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.yml index 4ff86a623f..1143ab375f 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-less_than_srv_records.yml @@ -13,3 +13,4 @@ hosts: options: srvMaxHosts: 1 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.json b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.json index 241a901c64..e232edb9eb 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.json @@ -13,5 +13,6 @@ "replicaSet": "repl0", "srvMaxHosts": 0, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.yml index 0df9fb49f4..81e92a09b1 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero-txt.yml @@ -13,3 +13,4 @@ options: replicaSet: repl0 srvMaxHosts: 0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.json b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.json index c68610a201..3421a35a3d 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.json @@ -13,5 +13,6 @@ "replicaSet": "repl0", "srvMaxHosts": 0, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.yml b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.yml index 3092889205..d095b6d677 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/srvMaxHosts-zero.yml @@ -13,3 +13,4 @@ options: replicaSet: repl0 srvMaxHosts: 0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.json b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.json index 66028310a6..43efcc6310 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.json @@ -12,5 +12,6 @@ "options": { "replicaSet": "repl0", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.yml b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.yml index 61d38b5e82..c3f7156dc7 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-default-port.yml @@ -9,3 +9,4 @@ hosts: options: replicaSet: repl0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.json b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.json index 4900f7cff1..f6e8e415a7 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.json @@ -12,5 +12,6 @@ "options": { "replicaSet": "repl0", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.yml b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.yml index 7185f52cd6..fe66308111 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/two-results-nonstandard-port.yml @@ -9,3 +9,4 @@ hosts: options: replicaSet: repl0 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.json b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.json index 0ebc737bd5..3d84cfe446 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.json @@ -12,5 +12,6 @@ "replicaSet": "repl0", "authSource": "thisDB", "ssl": false - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.yml b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.yml index 2a922aa234..8d1a46c396 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-ssl-option.yml @@ -9,3 +9,4 @@ options: replicaSet: repl0 authSource: thisDB ssl: false +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.json b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.json index 2626ba6083..1a5a240680 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.json @@ -12,5 +12,6 @@ "replicaSet": "repl0", "authSource": "otherDB", "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.yml b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.yml index a9015599e7..200ac6803e 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/txt-record-with-overridden-uri-option.yml @@ -9,3 +9,4 @@ options: replicaSet: repl0 authSource: otherDB ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.json b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.json index 32710d75f7..c5513a0dad 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.json +++ b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.json @@ -15,5 +15,6 @@ }, "parsed_options": { "auth_database": "adminDB" - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.yml b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.yml index fb714bde0e..012e9a023e 100644 --- a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.yml +++ b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-admin-database.yml @@ -11,3 +11,4 @@ options: ssl: true parsed_options: auth_database: adminDB +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.json b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.json new file mode 100644 index 0000000000..872f997cc7 --- /dev/null +++ b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.json @@ -0,0 +1,22 @@ +{ + "uri": "mongodb+srv://auser:apass@test1.test.build.10gen.cc/?replicaSet=repl0", + "seeds": [ + "localhost.test.build.10gen.cc:27017", + "localhost.test.build.10gen.cc:27018" + ], + "hosts": [ + "localhost:27017", + "localhost:27018", + "localhost:27019" + ], + "options": { + "replicaSet": "repl0", + "ssl": true + }, + "parsed_options": { + "user": "auser", + "password": "apass" + }, + "ping": false, + "comment": "Should preserve auth credentials" +} diff --git a/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.yml b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.yml new file mode 100644 index 0000000000..95f9b96426 --- /dev/null +++ b/testdata/initial-dns-seedlist-discovery/replica-set/uri-with-auth.yml @@ -0,0 +1,19 @@ +uri: "mongodb+srv://auser:apass@test1.test.build.10gen.cc/?replicaSet=repl0" +seeds: + - localhost.test.build.10gen.cc:27017 + - localhost.test.build.10gen.cc:27018 +hosts: + - localhost:27017 + - localhost:27018 + - localhost:27019 +options: + replicaSet: repl0 + ssl: true +parsed_options: + user: auser + password: apass +# Don't run a ping for URIs that include userinfo. Ping doesn't require authentication, so missing +# userinfo isn't a problem, but some drivers will fail handshake on a connection if userinfo is +# provided but incorrect. +ping: false +comment: Should preserve auth credentials diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.json b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.json index 46390726f0..7d2f9a6bf8 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.json @@ -12,5 +12,6 @@ "options": { "srvMaxHosts": 2, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.yml b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.yml index 89692308b6..84aeac9aec 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-equal_to_srv_records.yml @@ -11,3 +11,4 @@ hosts: options: srvMaxHosts: 2 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.json b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.json index e02d72bf28..452c7b54db 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.json @@ -11,5 +11,6 @@ "options": { "srvMaxHosts": 3, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.yml b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.yml index 860dfacbb4..8195fd0d33 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-greater_than_srv_records.yml @@ -10,3 +10,4 @@ hosts: options: srvMaxHosts: 3 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.json b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.json index fdcc1692c0..cd3bf65117 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.json +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.json @@ -5,5 +5,6 @@ "options": { "srvMaxHosts": 1, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.yml b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.yml index 31849524df..e33429b67a 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.yml +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-less_than_srv_records.yml @@ -8,3 +8,4 @@ numHosts: 1 options: srvMaxHosts: 1 ssl: true +ping: true diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.json b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.json index 10ab9e656d..f289628c9c 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.json +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.json @@ -11,5 +11,6 @@ "options": { "srvMaxHosts": 0, "ssl": true - } + }, + "ping": true } diff --git a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.yml b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.yml index 9043409dfd..5692ed09b8 100644 --- a/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.yml +++ b/testdata/initial-dns-seedlist-discovery/sharded/srvMaxHosts-zero.yml @@ -9,3 +9,4 @@ hosts: options: srvMaxHosts: 0 ssl: true +ping: true diff --git a/testdata/retryable-writes/unified/insertOne-noWritesPerformedError.yml b/testdata/retryable-writes/unified/insertOne-noWritesPerformedError.yml new file mode 100644 index 0000000000..3295d153dd --- /dev/null +++ b/testdata/retryable-writes/unified/insertOne-noWritesPerformedError.yml @@ -0,0 +1,54 @@ +description: "retryable-writes insertOne noWritesPerformedErrors" + +schemaVersion: "1.0" + +runOnRequirements: + - minServerVersion: "6.0" + topologies: [ replicaset ] + +createEntities: + - client: + id: &client0 client0 + useMultipleMongoses: false + observeEvents: [ commandFailedEvent ] + - database: + id: &database0 database0 + client: *client0 + databaseName: &databaseName retryable-writes-tests + - collection: + id: &collection0 collection0 + database: *database0 + collectionName: &collectionName no-writes-performed-collection + +tests: + - description: "InsertOne fails after NoWritesPerformed error" + operations: + - name: failPoint + object: testRunner + arguments: + client: *client0 + failPoint: + configureFailPoint: failCommand + mode: + times: 2 + data: + failCommands: + - insert + errorCode: 64 + errorLabels: + - NoWritesPerformed + - RetryableWriteError + - name: insertOne + object: *collection0 + arguments: + document: + x: 1 + expectError: + errorCode: 64 + errorLabelsContain: + - NoWritesPerformed + - RetryableWriteError + outcome: + - collectionName: *collectionName + databaseName: *databaseName + documents: [] diff --git a/testdata/retryable-writes/unified/insertOne-noWritesPerformedErrors.json b/testdata/retryable-writes/unified/insertOne-noWritesPerformedErrors.json new file mode 100644 index 0000000000..3194e91c5c --- /dev/null +++ b/testdata/retryable-writes/unified/insertOne-noWritesPerformedErrors.json @@ -0,0 +1,90 @@ +{ + "description": "retryable-writes insertOne noWritesPerformedErrors", + "schemaVersion": "1.0", + "runOnRequirements": [ + { + "minServerVersion": "6.0", + "topologies": [ + "replicaset" + ] + } + ], + "createEntities": [ + { + "client": { + "id": "client0", + "useMultipleMongoses": false, + "observeEvents": [ + "commandFailedEvent" + ] + } + }, + { + "database": { + "id": "database0", + "client": "client0", + "databaseName": "retryable-writes-tests" + } + }, + { + "collection": { + "id": "collection0", + "database": "database0", + "collectionName": "no-writes-performed-collection" + } + } + ], + "tests": [ + { + "description": "InsertOne fails after NoWritesPerformed error", + "operations": [ + { + "name": "failPoint", + "object": "testRunner", + "arguments": { + "client": "client0", + "failPoint": { + "configureFailPoint": "failCommand", + "mode": { + "times": 2 + }, + "data": { + "failCommands": [ + "insert" + ], + "errorCode": 64, + "errorLabels": [ + "NoWritesPerformed", + "RetryableWriteError" + ] + } + } + } + }, + { + "name": "insertOne", + "object": "collection0", + "arguments": { + "document": { + "x": 1 + } + }, + "expectError": { + "errorCode": 64, + "errorLabelsContain": [ + "NoWritesPerformed", + "RetryableWriteError" + ] + } + } + ], + "outcome": [ + { + "collectionName": "no-writes-performed-collection", + "databaseName": "retryable-writes-tests", + "documents": [] + } + ] + } + ] +} diff --git a/x/mongo/driver/operation.go b/x/mongo/driver/operation.go index 6324e95119..bbc25ecf22 100644 --- a/x/mongo/driver/operation.go +++ b/x/mongo/driver/operation.go @@ -59,8 +59,9 @@ type RetryablePoolError interface { Retryable() bool } -// LabeledError is an error that can have error labels added to it. -type LabeledError interface { +// labeledError is an error that can have error labels added to it. +type labeledError interface { + error HasErrorLabel(string) bool } @@ -398,9 +399,19 @@ func (op Operation) Execute(ctx context.Context) error { // Set the previous indefinite error to be returned in any case where a retryable write error does not have a // NoWritesPerfomed label (the definite case). switch err := err.(type) { - case LabeledError: + case labeledError: + // If the "prevIndefiniteErr" is nil, then the current error is the first error encountered + // during the retry attempt cycle. We must persist the first error in the case where all + // following errors are labeled "NoWritesPerformed", which would otherwise raise nil as the + // error. + if prevIndefiniteErr == nil { + prevIndefiniteErr = err + } + + // If the error is not labeled NoWritesPerformed and is retryable, then set the previous + // indefinite error to be the current error. if !err.HasErrorLabel(NoWritesPerformed) && err.HasErrorLabel(RetryableWriteError) { - prevIndefiniteErr = err.(error) + prevIndefiniteErr = err } } @@ -595,6 +606,13 @@ func (op Operation) Execute(ctx context.Context) error { finishedInfo.cmdErr = err op.publishFinishedEvent(ctx, finishedInfo) + // prevIndefiniteErrorIsSet is "true" if the "err" variable has been set to the "prevIndefiniteErr" in + // a case in the switch statement below. + var prevIndefiniteErrIsSet bool + + // TODO(GODRIVER-2579): When refactoring the "Execute" method, consider creating a separate method for the + // error handling logic below. This will remove the necessity of the "checkError" goto label. + checkError: var perr error switch tt := err.(type) { case WriteCommandError: @@ -627,9 +645,13 @@ func (op Operation) Execute(ctx context.Context) error { } // If the error is no longer retryable and has the NoWritesPerformed label, then we should - // return the previous indefinite error. - if tt.HasErrorLabel(NoWritesPerformed) { - return prevIndefiniteErr + // set the error to the "previous indefinite error" unless the current error is already the + // "previous indefinite error". After reseting, repeat the error check. + if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet { + err = prevIndefiniteErr + prevIndefiniteErrIsSet = true + + goto checkError } // If the operation isn't being retried, process the response @@ -720,9 +742,13 @@ func (op Operation) Execute(ctx context.Context) error { } // If the error is no longer retryable and has the NoWritesPerformed label, then we should - // return the previous indefinite error. - if tt.HasErrorLabel(NoWritesPerformed) { - return prevIndefiniteErr + // set the error to the "previous indefinite error" unless the current error is already the + // "previous indefinite error". After reseting, repeat the error check. + if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet { + err = prevIndefiniteErr + prevIndefiniteErrIsSet = true + + goto checkError } // If the operation isn't being retried, process the response diff --git a/x/mongo/driver/topology/server.go b/x/mongo/driver/topology/server.go index bdd9035ead..d416f6c195 100644 --- a/x/mongo/driver/topology/server.go +++ b/x/mongo/driver/topology/server.go @@ -520,6 +520,7 @@ func (s *Server) update() { } } + timeoutCnt := 0 for { // Check if the server is disconnecting. Even if waitForNextCheck has already read from the done channel, we // can safely read from it again because Disconnect closes the channel. @@ -545,18 +546,42 @@ func (s *Server) update() { continue } - // Must hold the processErrorLock while updating the server description and clearing the - // pool. Not holding the lock leads to possible out-of-order processing of pool.clear() and - // pool.ready() calls from concurrent server description updates. - s.processErrorLock.Lock() - s.updateDescription(desc) - if err := desc.LastError; err != nil { - // Clear the pool once the description has been updated to Unknown. Pass in a nil service ID to clear - // because the monitoring routine only runs for non-load balanced deployments in which servers don't return - // IDs. - s.pool.clear(err, nil) + if isShortcut := func() bool { + // Must hold the processErrorLock while updating the server description and clearing the + // pool. Not holding the lock leads to possible out-of-order processing of pool.clear() and + // pool.ready() calls from concurrent server description updates. + s.processErrorLock.Lock() + defer s.processErrorLock.Unlock() + + s.updateDescription(desc) + // Retry after the first timeout before clearing the pool in case of a FAAS pause as + // described in GODRIVER-2577. + if err := unwrapConnectionError(desc.LastError); err != nil && timeoutCnt < 1 { + if err == context.Canceled || err == context.DeadlineExceeded { + timeoutCnt++ + // We want to immediately retry on timeout error. Continue to next loop. + return true + } + if err, ok := err.(net.Error); ok && err.Timeout() { + timeoutCnt++ + // We want to immediately retry on timeout error. Continue to next loop. + return true + } + } + if err := desc.LastError; err != nil { + // Clear the pool once the description has been updated to Unknown. Pass in a nil service ID to clear + // because the monitoring routine only runs for non-load balanced deployments in which servers don't return + // IDs. + s.pool.clear(err, nil) + } + // We're either not handling a timeout error, or we just handled the 2nd consecutive + // timeout error. In either case, reset the timeout count to 0 and return false to + // continue the normal check process. + timeoutCnt = 0 + return false + }(); isShortcut { + continue } - s.processErrorLock.Unlock() // If the server supports streaming or we're already streaming, we want to move to streaming the next response // without waiting. If the server has transitioned to Unknown from a network error, we want to do another @@ -707,19 +732,31 @@ func (s *Server) check() (description.Server, error) { var err error var durationNanos int64 - // Create a new connection if this is the first check, the connection was closed after an error during the previous - // check, or the previous check was cancelled. + start := time.Now() if s.conn == nil || s.conn.closed() || s.checkWasCancelled() { + // Create a new connection if this is the first check, the connection was closed after an error during the previous + // check, or the previous check was cancelled. + isNilConn := s.conn == nil + if !isNilConn { + s.publishServerHeartbeatStartedEvent(s.conn.ID(), false) + } // Create a new connection and add it's handshake RTT as a sample. err = s.setupHeartbeatConnection() + durationNanos = time.Since(start).Nanoseconds() if err == nil { // Use the description from the connection handshake as the value for this check. s.rttMonitor.addSample(s.conn.helloRTT) descPtr = &s.conn.desc + if !isNilConn { + s.publishServerHeartbeatSucceededEvent(s.conn.ID(), durationNanos, s.conn.desc, false) + } + } else { + err = unwrapConnectionError(err) + if !isNilConn { + s.publishServerHeartbeatFailedEvent(s.conn.ID(), durationNanos, err, false) + } } - } - - if descPtr == nil && err == nil { + } else { // An existing connection is being used. Use the server description properties to execute the right heartbeat. // Wrap conn in a type that implements driver.StreamerConnection. @@ -729,7 +766,6 @@ func (s *Server) check() (description.Server, error) { streamable := previousDescription.TopologyVersion != nil s.publishServerHeartbeatStartedEvent(s.conn.ID(), s.conn.getCurrentlyStreaming() || streamable) - start := time.Now() switch { case s.conn.getCurrentlyStreaming(): // The connection is already in a streaming state, so we stream the next response. diff --git a/x/mongo/driver/topology/server_test.go b/x/mongo/driver/topology/server_test.go index 9850665db1..ecb001e311 100644 --- a/x/mongo/driver/topology/server_test.go +++ b/x/mongo/driver/topology/server_test.go @@ -11,8 +11,12 @@ package topology import ( "context" + "crypto/tls" + "crypto/x509" "errors" + "io/ioutil" "net" + "os" "runtime" "sync" "sync/atomic" @@ -49,6 +53,144 @@ func (cncd *channelNetConnDialer) DialContext(_ context.Context, _, _ string) (n return cnc, nil } +type errorQueue struct { + errors []error + mutex sync.Mutex +} + +func (eq *errorQueue) head() error { + eq.mutex.Lock() + defer eq.mutex.Unlock() + if len(eq.errors) > 0 { + return eq.errors[0] + } + return nil +} + +func (eq *errorQueue) dequeue() bool { + eq.mutex.Lock() + defer eq.mutex.Unlock() + if len(eq.errors) > 0 { + eq.errors = eq.errors[1:] + return true + } + return false +} + +type timeoutConn struct { + net.Conn + errors *errorQueue +} + +func (c *timeoutConn) Read(b []byte) (int, error) { + n, err := 0, c.errors.head() + if err == nil { + n, err = c.Conn.Read(b) + } + return n, err +} + +func (c *timeoutConn) Write(b []byte) (int, error) { + n, err := 0, c.errors.head() + if err == nil { + n, err = c.Conn.Write(b) + } + return n, err +} + +type timeoutDialer struct { + Dialer + errors *errorQueue +} + +func (d *timeoutDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) { + c, e := d.Dialer.DialContext(ctx, network, address) + + if caFile := os.Getenv("MONGO_GO_DRIVER_CA_FILE"); len(caFile) > 0 { + pem, err := ioutil.ReadFile(caFile) + if err != nil { + return nil, err + } + + ca := x509.NewCertPool() + if !ca.AppendCertsFromPEM(pem) { + return nil, errors.New("unable to load CA file") + } + + config := &tls.Config{ + InsecureSkipVerify: true, + RootCAs: ca, + } + c = tls.Client(c, config) + } + return &timeoutConn{c, d.errors}, e +} + +// TestServerHeartbeatTimeout tests timeout retry for GODRIVER-2577. +func TestServerHeartbeatTimeout(t *testing.T) { + networkTimeoutError := &net.DNSError{ + IsTimeout: true, + } + + testCases := []struct { + desc string + ioErrors []error + expectPoolCleared bool + }{ + { + desc: "one single timeout should not clear the pool", + ioErrors: []error{nil, networkTimeoutError, nil, networkTimeoutError, nil}, + expectPoolCleared: false, + }, + { + desc: "continuous timeouts should clear the pool", + ioErrors: []error{nil, networkTimeoutError, networkTimeoutError, nil}, + expectPoolCleared: true, + }, + } + for _, tc := range testCases { + tc := tc + t.Run(tc.desc, func(t *testing.T) { + t.Parallel() + + var wg sync.WaitGroup + wg.Add(1) + + errors := &errorQueue{errors: tc.ioErrors} + tpm := monitor.NewTestPoolMonitor() + server := NewServer( + address.Address("localhost:27017"), + primitive.NewObjectID(), + WithConnectionPoolMonitor(func(*event.PoolMonitor) *event.PoolMonitor { + return tpm.PoolMonitor + }), + WithConnectionOptions(func(opts ...ConnectionOption) []ConnectionOption { + return append(opts, + WithDialer(func(d Dialer) Dialer { + var dialer net.Dialer + return &timeoutDialer{&dialer, errors} + })) + }), + WithServerMonitor(func(*event.ServerMonitor) *event.ServerMonitor { + return &event.ServerMonitor{ + ServerHeartbeatStarted: func(e *event.ServerHeartbeatStartedEvent) { + if !errors.dequeue() { + wg.Done() + } + }, + } + }), + WithHeartbeatInterval(func(time.Duration) time.Duration { + return 200 * time.Millisecond + }), + ) + require.NoError(t, server.Connect(nil)) + wg.Wait() + assert.Equal(t, tc.expectPoolCleared, tpm.IsPoolCleared(), "expected pool cleared to be %v but was %v", tc.expectPoolCleared, tpm.IsPoolCleared()) + }) + } +} + // TestServerConnectionTimeout tests how different timeout errors are handled during connection // creation and server handshake. func TestServerConnectionTimeout(t *testing.T) {