diff --git a/mongo/integration/retryable_writes_prose_test.go b/mongo/integration/retryable_writes_prose_test.go index 088344db97..40710fa4e8 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/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