Skip to content

Commit 08fd347

Browse files
mbroadstmbroadst
committed
fix: writes within transactions are not retryable
Unless a write command is `commitTransaction` or `abortTransaction` it should not receive a `RetryableWriteError` label on command failure. NODE-2594
1 parent f96a97f commit 08fd347

File tree

5 files changed

+216
-24
lines changed

5 files changed

+216
-24
lines changed

‎lib/sdam/server.js

+17-8
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ const {
2929
isNodeShuttingDownError,
3030
isNetworkErrorBeforeHandshake
3131
} = require('../error');
32+
const { isTransactionCommand } = require('../transactions');
3233

3334
// Used for filtering out fields for logging
3435
const DEBUG_FIELDS = [
@@ -267,7 +268,7 @@ class Server extends EventEmitter {
267268
return cb(err);
268269
}
269270

270-
conn.command(ns, cmd, options, makeOperationHandler(this, conn, options, cb));
271+
conn.command(ns, cmd, options, makeOperationHandler(this, conn, cmd, options, cb));
271272
}, callback);
272273
}
273274

@@ -292,7 +293,7 @@ class Server extends EventEmitter {
292293
return cb(err);
293294
}
294295

295-
conn.query(ns, cmd, cursorState, options, makeOperationHandler(this, conn, options, cb));
296+
conn.query(ns, cmd, cursorState, options, makeOperationHandler(this, conn, cmd, options, cb));
296297
}, callback);
297298
}
298299

@@ -322,7 +323,7 @@ class Server extends EventEmitter {
322323
cursorState,
323324
batchSize,
324325
options,
325-
makeOperationHandler(this, conn, options, cb)
326+
makeOperationHandler(this, conn, null, options, cb)
326327
);
327328
}, callback);
328329
}
@@ -349,7 +350,7 @@ class Server extends EventEmitter {
349350
return cb(err);
350351
}
351352

352-
conn.killCursors(ns, cursorState, makeOperationHandler(this, conn, undefined, cb));
353+
conn.killCursors(ns, cursorState, makeOperationHandler(this, conn, null, undefined, cb));
353354
}, callback);
354355
}
355356

@@ -466,7 +467,7 @@ function executeWriteOperation(args, options, callback) {
466467
return cb(err);
467468
}
468469

469-
conn[op](ns, ops, options, makeOperationHandler(server, conn, options, cb));
470+
conn[op](ns, ops, options, makeOperationHandler(server, conn, ops, options, cb));
470471
}, callback);
471472
}
472473

@@ -496,7 +497,11 @@ function shouldHandleStateChangeError(server, err) {
496497
return compareTopologyVersion(stv, etv) < 0;
497498
}
498499

499-
function makeOperationHandler(server, connection, options, callback) {
500+
function inActiveTransaction(session, cmd) {
501+
return session && session.inTransaction() && !isTransactionCommand(cmd);
502+
}
503+
504+
function makeOperationHandler(server, connection, cmd, options, callback) {
500505
const session = options && options.session;
501506

502507
return function handleOperationResult(err, result) {
@@ -506,7 +511,7 @@ function makeOperationHandler(server, connection, options, callback) {
506511
session.serverSession.isDirty = true;
507512
}
508513

509-
if (supportsRetryableWrites(server)) {
514+
if (supportsRetryableWrites(server) && !inActiveTransaction(session, cmd)) {
510515
err.addErrorLabel('RetryableWriteError');
511516
}
512517

@@ -516,7 +521,11 @@ function makeOperationHandler(server, connection, options, callback) {
516521
}
517522
} else {
518523
// if pre-4.4 server, then add error label if its a retryable write error
519-
if (maxWireVersion(server) < 9 && isRetryableWriteError(err)) {
524+
if (
525+
maxWireVersion(server) < 9 &&
526+
isRetryableWriteError(err) &&
527+
!inActiveTransaction(session, cmd)
528+
) {
520529
err.addErrorLabel('RetryableWriteError');
521530
}
522531

‎test/functional/core/operations.test.js

+12-4
Original file line numberDiff line numberDiff line change
@@ -298,7 +298,9 @@ describe('Operation tests', function() {
298298
},
299299
function(insertErr, insertResults) {
300300
expect(insertErr).to.be.null;
301-
expect(insertResults.result.n).to.equal(3);
301+
expect(insertResults)
302+
.nested.property('result.n')
303+
.to.equal(3);
302304

303305
// Execute find
304306
var cursor = _server.cursor(f('%s.inserts10', self.configuration.db), {
@@ -310,16 +312,22 @@ describe('Operation tests', function() {
310312
// Execute next
311313
cursor._next(function(cursorErr, cursorD) {
312314
expect(cursorErr).to.be.null;
313-
expect(cursorD.a).to.equal(1);
315+
expect(cursorD)
316+
.property('a')
317+
.to.equal(1);
314318

315319
// Execute next
316320
cursor._next(function(secondCursorErr, secondCursorD) {
317321
expect(secondCursorErr).to.be.null;
318-
expect(secondCursorD.a).to.equal(2);
322+
expect(secondCursorD)
323+
.property('a')
324+
.to.equal(2);
319325

320326
cursor._next(function(thirdCursorErr, thirdCursorD) {
321327
expect(thirdCursorErr).to.be.null;
322-
expect(thirdCursorD.a).to.equal(3);
328+
expect(thirdCursorD)
329+
.property('a')
330+
.to.equal(3);
323331

324332
// Destroy the server connection
325333
_server.destroy(done);

‎test/spec/transactions/README.rst

+14-6
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,10 @@ The ``failCommand`` fail point may be configured like so::
4141
failCommands: ["commandName", "commandName2"],
4242
closeConnection: <true|false>,
4343
errorCode: <Number>,
44-
writeConcernError: <document>
44+
writeConcernError: <document>,
45+
appName: <string>,
46+
blockConnection: <true|false>,
47+
blockTimeMS: <Number>,
4548
}
4649
});
4750

@@ -66,10 +69,13 @@ control the fail point's behavior. ``failCommand`` supports the following
6669
- ``errorCode``: Integer option, which is unset by default. If set, the command
6770
will not be executed and the specified command error code will be returned as
6871
a command error.
69-
- ``writeConcernError``: A document, which is unset by default. If set, the
70-
server will return this document in the "writeConcernError" field. This
71-
failure response only applies to commands that support write concern and
72-
happens *after* the command finishes (regardless of success or failure).
72+
- ``appName``: A string to filter which MongoClient should be affected by
73+
the failpoint. `New in mongod 4.4.0-rc2 <https://jira.mongodb.org/browse/SERVER-47195>`_.
74+
- ``blockConnection``: Whether the server should block the affected commands.
75+
Default false.
76+
- ``blockTimeMS``: The number of milliseconds the affect commands should be
77+
blocked for. Required when blockConnection is true.
78+
`New in mongod 4.3.4 <https://jira.mongodb.org/browse/SERVER-41070>`_.
7379

7480
Test Format
7581
===========
@@ -175,7 +181,7 @@ Each YAML file has the following keys:
175181
- ``collection``:
176182

177183
- ``data``: The data that should exist in the collection after the
178-
operations have run.
184+
operations have run, sorted by "_id".
179185

180186
Use as Integration Tests
181187
========================
@@ -299,6 +305,8 @@ Then for each element in ``tests``:
299305
latest data by using **primary read preference** with
300306
**local read concern** even when the MongoClient is configured with
301307
another read preference or read concern.
308+
Note the server does not guarantee that documents returned by a find
309+
command will be in inserted order. This find MUST sort by ``{_id:1}``.
302310

303311
.. _SERVER-38335: https://jira.mongodb.org/browse/SERVER-38335
304312

‎test/spec/transactions/error-labels.json

+105-1
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,7 @@
134134
"TransientTransactionError"
135135
],
136136
"errorLabelsOmit": [
137+
"RetryableWriteError",
137138
"UnknownTransactionCommitResult"
138139
]
139140
}
@@ -223,6 +224,7 @@
223224
"TransientTransactionError"
224225
],
225226
"errorLabelsOmit": [
227+
"RetryableWriteError",
226228
"UnknownTransactionCommitResult"
227229
]
228230
}
@@ -312,6 +314,7 @@
312314
"TransientTransactionError"
313315
],
314316
"errorLabelsOmit": [
317+
"RetryableWriteError",
315318
"UnknownTransactionCommitResult"
316319
]
317320
}
@@ -462,7 +465,7 @@
462465
}
463466
},
464467
{
465-
"description": "add TransientTransactionError label to connection errors",
468+
"description": "add TransientTransactionError label to connection errors, but do not add RetryableWriteError label",
466469
"failPoint": {
467470
"configureFailPoint": "failCommand",
468471
"mode": {
@@ -497,6 +500,7 @@
497500
"TransientTransactionError"
498501
],
499502
"errorLabelsOmit": [
503+
"RetryableWriteError",
500504
"UnknownTransactionCommitResult"
501505
]
502506
}
@@ -512,6 +516,7 @@
512516
"TransientTransactionError"
513517
],
514518
"errorLabelsOmit": [
519+
"RetryableWriteError",
515520
"UnknownTransactionCommitResult"
516521
]
517522
}
@@ -534,6 +539,7 @@
534539
"TransientTransactionError"
535540
],
536541
"errorLabelsOmit": [
542+
"RetryableWriteError",
537543
"UnknownTransactionCommitResult"
538544
]
539545
}
@@ -550,6 +556,7 @@
550556
"TransientTransactionError"
551557
],
552558
"errorLabelsOmit": [
559+
"RetryableWriteError",
553560
"UnknownTransactionCommitResult"
554561
]
555562
}
@@ -1098,6 +1105,103 @@
10981105
}
10991106
}
11001107
},
1108+
{
1109+
"description": "do not add RetryableWriteError label to writeConcernError ShutdownInProgress that occurs within transaction",
1110+
"failPoint": {
1111+
"configureFailPoint": "failCommand",
1112+
"mode": {
1113+
"times": 1
1114+
},
1115+
"data": {
1116+
"failCommands": [
1117+
"insert"
1118+
],
1119+
"writeConcernError": {
1120+
"code": 91,
1121+
"errmsg": "Replication is being shut down"
1122+
}
1123+
}
1124+
},
1125+
"operations": [
1126+
{
1127+
"name": "startTransaction",
1128+
"object": "session0",
1129+
"arguments": {
1130+
"options": {
1131+
"writeConcern": {
1132+
"w": "majority"
1133+
}
1134+
}
1135+
}
1136+
},
1137+
{
1138+
"name": "insertOne",
1139+
"object": "collection",
1140+
"arguments": {
1141+
"session": "session0",
1142+
"document": {
1143+
"_id": 1
1144+
}
1145+
},
1146+
"result": {
1147+
"errorLabelsContain": [],
1148+
"errorLabelsOmit": [
1149+
"RetryableWriteError",
1150+
"TransientTransactionError",
1151+
"UnknownTransactionCommitResult"
1152+
]
1153+
}
1154+
},
1155+
{
1156+
"name": "abortTransaction",
1157+
"object": "session0"
1158+
}
1159+
],
1160+
"expectations": [
1161+
{
1162+
"command_started_event": {
1163+
"command": {
1164+
"insert": "test",
1165+
"documents": [
1166+
{
1167+
"_id": 1
1168+
}
1169+
],
1170+
"ordered": true,
1171+
"readConcern": null,
1172+
"lsid": "session0",
1173+
"txnNumber": {
1174+
"$numberLong": "1"
1175+
},
1176+
"startTransaction": true,
1177+
"autocommit": false
1178+
},
1179+
"command_name": "insert",
1180+
"database_name": "transaction-tests"
1181+
}
1182+
},
1183+
{
1184+
"command_started_event": {
1185+
"command": {
1186+
"abortTransaction": 1,
1187+
"lsid": "session0",
1188+
"txnNumber": {
1189+
"$numberLong": "1"
1190+
},
1191+
"startTransaction": null,
1192+
"autocommit": false
1193+
},
1194+
"command_name": "abortTransaction",
1195+
"database_name": "admin"
1196+
}
1197+
}
1198+
],
1199+
"outcome": {
1200+
"collection": {
1201+
"data": []
1202+
}
1203+
}
1204+
},
11011205
{
11021206
"description": "add UnknownTransactionCommitResult label to writeConcernError WriteConcernFailed",
11031207
"failPoint": {

0 commit comments

Comments
 (0)