From 026a71c19c3f4604e8bc134e74dede28f175c063 Mon Sep 17 00:00:00 2001 From: Benety Goh Date: Mon, 24 Jul 2017 12:06:52 -0400 Subject: SERVER-30049 applyOperation_inlock() allows exceptions from Collection::insertDocument() to percolate to caller This specifically addresses the case where a WriteConflictException is thrown from insertDocument(). Converting this exception into a status will not allow the write conflict retry loops in callers to work properly. (cherry picked from commit f0d41183e735546c83ff96d7d5afc11b9c94cb9f) --- .../apply_ops_insert_write_conflict_nonatomic.js | 10 +++ .../libs/apply_ops_insert_write_conflict.js | 84 ++++++++++++++++++++++ src/mongo/db/repl/oplog.cpp | 9 +-- 3 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 jstests/replsets/apply_ops_insert_write_conflict_nonatomic.js create mode 100644 jstests/replsets/libs/apply_ops_insert_write_conflict.js diff --git a/jstests/replsets/apply_ops_insert_write_conflict_nonatomic.js b/jstests/replsets/apply_ops_insert_write_conflict_nonatomic.js new file mode 100644 index 00000000000..2e91de8637a --- /dev/null +++ b/jstests/replsets/apply_ops_insert_write_conflict_nonatomic.js @@ -0,0 +1,10 @@ +(function() { + 'use strict'; + + load("jstests/replsets/libs/apply_ops_insert_write_conflict.js"); + + new ApplyOpsInsertWriteConflictTest({ + testName: 'apply_ops_insert_write_conflict_nonatomic', + atomic: false + }).run(); +}()); diff --git a/jstests/replsets/libs/apply_ops_insert_write_conflict.js b/jstests/replsets/libs/apply_ops_insert_write_conflict.js new file mode 100644 index 00000000000..88b299c08b6 --- /dev/null +++ b/jstests/replsets/libs/apply_ops_insert_write_conflict.js @@ -0,0 +1,84 @@ +/** + * Sets up a test for WriteConflictException handling in applyOps with an insert workload. + */ +var ApplyOpsInsertWriteConflictTest = function(options) { + 'use strict'; + + if (!(this instanceof ApplyOpsInsertWriteConflictTest)) { + return new ApplyOpsInsertWriteConflictTest(options); + } + + // Capture the 'this' reference + var self = this; + + self.options = options; + + /** + * Runs the test. + */ + this.run = function() { + var options = this.options; + + var replTest = new ReplSetTest({nodes: 1}); + replTest.startSet(); + replTest.initiate(); + + var primary = replTest.getPrimary(); + var primaryDB = primary.getDB('test'); + + var t = primaryDB.getCollection(options.testName); + t.drop(); + + assert.commandWorked(primaryDB.createCollection(t.getName())); + + var numOps = 1000; + var ops = Array(numOps).fill('ignored').map((unused, i) => { + return {op: 'i', ns: t.getFullName(), o: {_id: i}}; + }); + + if (!options.atomic) { + // Adding a command to the list of operations to prevent the applyOps command from + // applying + // all the operations atomically. + ops.push({ns: "test.$cmd", op: "c", o: {applyOps: []}}); + numOps++; + } + + // Probabilities for WCE are chosen based on empirical testing. + // The probability for WCE during an atomic applyOps should be much smaller than that for + // the non-atomic case because we have to attempt to re-apply the entire batch of 'numOps' + // operations on WCE in the atomic case. + var probability = (options.atomic ? 0.1 : 5.0) / numOps; + + // Set up failpoint to trigger WriteConflictException during write operations. + assert.commandWorked( + primaryDB.adminCommand({setParameter: 1, traceWriteConflictExceptions: true})); + assert.commandWorked(primaryDB.adminCommand({ + configureFailPoint: 'WTWriteConflictException', + mode: {activationProbability: probability} + })); + + // This logs each operation being applied. + var previousLogLevel = + assert.commandWorked(primaryDB.setLogLevel(3, 'replication')).was.replication.verbosity; + + var applyOpsResult = primaryDB.adminCommand({applyOps: ops}); + + // Reset log level. + primaryDB.setLogLevel(previousLogLevel, 'replication'); + + assert.eq( + numOps, + applyOpsResult.applied, + 'number of operations applied did not match list of generated insert operations. ' + + 'applyOps result: ' + tojson(applyOpsResult)); + applyOpsResult.results.forEach((operationSucceeded, i) => { + assert(operationSucceeded, + 'applyOps failed: operation with index ' + i + ' failed: operation: ' + + tojson(ops[i], '', true) + '. applyOps result: ' + tojson(applyOpsResult)); + }); + assert.commandWorked(applyOpsResult); + + replTest.stopSet(); + }; +}; diff --git a/src/mongo/db/repl/oplog.cpp b/src/mongo/db/repl/oplog.cpp index 69bc38cd3e5..43a1eba5042 100644 --- a/src/mongo/db/repl/oplog.cpp +++ b/src/mongo/db/repl/oplog.cpp @@ -838,7 +838,6 @@ Status applyOperation_inlock(OperationContext* txn, // 2. If okay, commit // 3. If not, do upsert (and commit) // 4. If both !Ok, return status - Status status{ErrorCodes::NotYetInitialized, ""}; // We cannot rely on a DuplicateKey error if we'repart of a larger transaction, because // that would require the transaction to abort. So instead, use upsert in that case. @@ -846,12 +845,8 @@ Status applyOperation_inlock(OperationContext* txn, if (!needToDoUpsert) { WriteUnitOfWork wuow(txn); - try { - OpDebug* const nullOpDebug = nullptr; - status = collection->insertDocument(txn, o, nullOpDebug, true); - } catch (DBException dbe) { - status = dbe.toStatus(); - } + OpDebug* const nullOpDebug = nullptr; + auto status = collection->insertDocument(txn, o, nullOpDebug, true); if (status.isOK()) { wuow.commit(); } else if (status == ErrorCodes::DuplicateKey) { -- cgit v1.2.1