Skip to content

Commit 19f3fb9

Browse files
prestonvasquezqingyang-hubenjirewiskevinAlbs
committed
GODRIVER-2651 Break NoWritesPerformed-Only Error Sequence (#1135)
Co-authored-by: Qingyang Hu <[email protected]> Co-authored-by: Benjamin Rewis <[email protected]> Co-authored-by: Kevin Albertson <[email protected]> Co-authored-by: Qingyang Hu <[email protected]> Co-authored-by: Benjamin Rewis <[email protected]>
1 parent 4803b59 commit 19f3fb9

File tree

4 files changed

+181
-11
lines changed

4 files changed

+181
-11
lines changed

‎mongo/integration/retryable_writes_prose_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ func TestRetryableWritesProse(t *testing.T) {
282282

283283
require.True(mt, secondFailPointConfigured)
284284

285-
// Assert that the "NotWritablePrimary" error is returned.
285+
// Assert that the "ShutdownInProgress" error is returned.
286286
require.True(mt, err.(mongo.WriteException).HasErrorCode(int(shutdownInProgressErrorCode)))
287287
})
288288
}

‎testdata/retryable-writes/unified/insertOne-noWritesPerformedError.yml

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
description: "retryable-writes insertOne noWritesPerformedErrors"
2+
3+
schemaVersion: "1.0"
4+
5+
runOnRequirements:
6+
- minServerVersion: "6.0"
7+
topologies: [ replicaset ]
8+
9+
createEntities:
10+
- client:
11+
id: &client0 client0
12+
useMultipleMongoses: false
13+
observeEvents: [ commandFailedEvent ]
14+
- database:
15+
id: &database0 database0
16+
client: *client0
17+
databaseName: &databaseName retryable-writes-tests
18+
- collection:
19+
id: &collection0 collection0
20+
database: *database0
21+
collectionName: &collectionName no-writes-performed-collection
22+
23+
tests:
24+
- description: "InsertOne fails after NoWritesPerformed error"
25+
operations:
26+
- name: failPoint
27+
object: testRunner
28+
arguments:
29+
client: *client0
30+
failPoint:
31+
configureFailPoint: failCommand
32+
mode:
33+
times: 2
34+
data:
35+
failCommands:
36+
- insert
37+
errorCode: 64
38+
errorLabels:
39+
- NoWritesPerformed
40+
- RetryableWriteError
41+
- name: insertOne
42+
object: *collection0
43+
arguments:
44+
document:
45+
x: 1
46+
expectError:
47+
errorCode: 64
48+
errorLabelsContain:
49+
- NoWritesPerformed
50+
- RetryableWriteError
51+
outcome:
52+
- collectionName: *collectionName
53+
databaseName: *databaseName
54+
documents: []

‎testdata/retryable-writes/unified/insertOne-noWritesPerformedErrors.json

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
{
2+
"description": "retryable-writes insertOne noWritesPerformedErrors",
3+
"schemaVersion": "1.0",
4+
"runOnRequirements": [
5+
{
6+
"minServerVersion": "6.0",
7+
"topologies": [
8+
"replicaset"
9+
]
10+
}
11+
],
12+
"createEntities": [
13+
{
14+
"client": {
15+
"id": "client0",
16+
"useMultipleMongoses": false,
17+
"observeEvents": [
18+
"commandFailedEvent"
19+
]
20+
}
21+
},
22+
{
23+
"database": {
24+
"id": "database0",
25+
"client": "client0",
26+
"databaseName": "retryable-writes-tests"
27+
}
28+
},
29+
{
30+
"collection": {
31+
"id": "collection0",
32+
"database": "database0",
33+
"collectionName": "no-writes-performed-collection"
34+
}
35+
}
36+
],
37+
"tests": [
38+
{
39+
"description": "InsertOne fails after NoWritesPerformed error",
40+
"operations": [
41+
{
42+
"name": "failPoint",
43+
"object": "testRunner",
44+
"arguments": {
45+
"client": "client0",
46+
"failPoint": {
47+
"configureFailPoint": "failCommand",
48+
"mode": {
49+
"times": 2
50+
},
51+
"data": {
52+
"failCommands": [
53+
"insert"
54+
],
55+
"errorCode": 64,
56+
"errorLabels": [
57+
"NoWritesPerformed",
58+
"RetryableWriteError"
59+
]
60+
}
61+
}
62+
}
63+
},
64+
{
65+
"name": "insertOne",
66+
"object": "collection0",
67+
"arguments": {
68+
"document": {
69+
"x": 1
70+
}
71+
},
72+
"expectError": {
73+
"errorCode": 64,
74+
"errorLabelsContain": [
75+
"NoWritesPerformed",
76+
"RetryableWriteError"
77+
]
78+
}
79+
}
80+
],
81+
"outcome": [
82+
{
83+
"collectionName": "no-writes-performed-collection",
84+
"databaseName": "retryable-writes-tests",
85+
"documents": []
86+
}
87+
]
88+
}
89+
]
90+
}

‎x/mongo/driver/operation.go

Lines changed: 36 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,9 @@ type RetryablePoolError interface {
5959
Retryable() bool
6060
}
6161

62-
// LabeledError is an error that can have error labels added to it.
63-
type LabeledError interface {
62+
// labeledError is an error that can have error labels added to it.
63+
type labeledError interface {
64+
error
6465
HasErrorLabel(string) bool
6566
}
6667

@@ -398,9 +399,19 @@ func (op Operation) Execute(ctx context.Context) error {
398399
// Set the previous indefinite error to be returned in any case where a retryable write error does not have a
399400
// NoWritesPerfomed label (the definite case).
400401
switch err := err.(type) {
401-
case LabeledError:
402+
case labeledError:
403+
// If the "prevIndefiniteErr" is nil, then the current error is the first error encountered
404+
// during the retry attempt cycle. We must persist the first error in the case where all
405+
// following errors are labeled "NoWritesPerformed", which would otherwise raise nil as the
406+
// error.
407+
if prevIndefiniteErr == nil {
408+
prevIndefiniteErr = err
409+
}
410+
411+
// If the error is not labeled NoWritesPerformed and is retryable, then set the previous
412+
// indefinite error to be the current error.
402413
if !err.HasErrorLabel(NoWritesPerformed) && err.HasErrorLabel(RetryableWriteError) {
403-
prevIndefiniteErr = err.(error)
414+
prevIndefiniteErr = err
404415
}
405416
}
406417

@@ -595,6 +606,13 @@ func (op Operation) Execute(ctx context.Context) error {
595606
finishedInfo.cmdErr = err
596607
op.publishFinishedEvent(ctx, finishedInfo)
597608

609+
// prevIndefiniteErrorIsSet is "true" if the "err" variable has been set to the "prevIndefiniteErr" in
610+
// a case in the switch statement below.
611+
var prevIndefiniteErrIsSet bool
612+
613+
// TODO(GODRIVER-2579): When refactoring the "Execute" method, consider creating a separate method for the
614+
// error handling logic below. This will remove the necessity of the "checkError" goto label.
615+
checkError:
598616
var perr error
599617
switch tt := err.(type) {
600618
case WriteCommandError:
@@ -627,9 +645,13 @@ func (op Operation) Execute(ctx context.Context) error {
627645
}
628646

629647
// If the error is no longer retryable and has the NoWritesPerformed label, then we should
630-
// return the previous indefinite error.
631-
if tt.HasErrorLabel(NoWritesPerformed) {
632-
return prevIndefiniteErr
648+
// set the error to the "previous indefinite error" unless the current error is already the
649+
// "previous indefinite error". After reseting, repeat the error check.
650+
if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet {
651+
err = prevIndefiniteErr
652+
prevIndefiniteErrIsSet = true
653+
654+
goto checkError
633655
}
634656

635657
// If the operation isn't being retried, process the response
@@ -720,9 +742,13 @@ func (op Operation) Execute(ctx context.Context) error {
720742
}
721743

722744
// If the error is no longer retryable and has the NoWritesPerformed label, then we should
723-
// return the previous indefinite error.
724-
if tt.HasErrorLabel(NoWritesPerformed) {
725-
return prevIndefiniteErr
745+
// set the error to the "previous indefinite error" unless the current error is already the
746+
// "previous indefinite error". After reseting, repeat the error check.
747+
if tt.HasErrorLabel(NoWritesPerformed) && !prevIndefiniteErrIsSet {
748+
err = prevIndefiniteErr
749+
prevIndefiniteErrIsSet = true
750+
751+
goto checkError
726752
}
727753

728754
// If the operation isn't being retried, process the response

0 commit comments

Comments
 (0)