From 3030a5a09426ececa6ec285fa124d774463c2905 Mon Sep 17 00:00:00 2001 From: Shane Harvey Date: Fri, 8 Feb 2019 14:39:54 -0800 Subject: [PATCH] PYTHON-1730 Use w:majority when retrying commitTransaction (#393) --- pymongo/client_session.py | 51 +++-- test/transactions/commit.json | 10 +- test/transactions/error-labels.json | 32 +++- test/transactions/insert.json | 190 +++++++++++++++++++ test/transactions/retryable-abort.json | 4 +- test/transactions/retryable-commit.json | 240 ++++++++++++++++++++++-- 6 files changed, 479 insertions(+), 48 deletions(-) diff --git a/pymongo/client_session.py b/pymongo/client_session.py index e404a41bf..64ecf0b59 100644 --- a/pymongo/client_session.py +++ b/pymongo/client_session.py @@ -379,7 +379,7 @@ class ClientSession(object): .. versionadded:: 3.7 """ self._check_ended() - + retry = False state = self._transaction.state if state is _TxnState.NONE: raise InvalidOperation("No transaction started") @@ -391,12 +391,13 @@ class ClientSession(object): raise InvalidOperation( "Cannot call commitTransaction after calling abortTransaction") elif state is _TxnState.COMMITTED: - # We're rerunning the commit, move the state back to "in progress" - # so that _in_transaction returns true. + # We're explicitly retrying the commit, move the state back to + # "in progress" so that _in_transaction returns true. self._transaction.state = _TxnState.IN_PROGRESS + retry = True try: - self._finish_transaction_with_retry("commitTransaction") + self._finish_transaction_with_retry("commitTransaction", retry) except ConnectionFailure as exc: # We do not know if the commit was successfully applied on the # server or if it satisfied the provided write concern, set the @@ -439,31 +440,29 @@ class ClientSession(object): "Cannot call abortTransaction after calling commitTransaction") try: - self._finish_transaction_with_retry("abortTransaction") + self._finish_transaction_with_retry("abortTransaction", False) except (OperationFailure, ConnectionFailure): # The transactions spec says to ignore abortTransaction errors. pass finally: self._transaction.state = _TxnState.ABORTED - def _finish_transaction(self, command_name): - with self._client._socket_for_writes(self) as sock_info: - return self._client.admin._command( - sock_info, - command_name, - session=self, - write_concern=self._transaction.opts.write_concern, - parse_write_concern_error=True) + def _finish_transaction_with_retry(self, command_name, explict_retry): + """Run commit or abort with one retry after any retryable error. - def _finish_transaction_with_retry(self, command_name): + :Parameters: + - `command_name`: Either "commitTransaction" or "abortTransaction". + - `explict_retry`: True when this is an explict commit retry attempt, + ie the application called session.commit_transaction() twice. + """ # This can be refactored with MongoClient._retry_with_session. try: - return self._finish_transaction(command_name) + return self._finish_transaction(command_name, explict_retry) except ServerSelectionTimeoutError: raise except ConnectionFailure as exc: try: - return self._finish_transaction(command_name) + return self._finish_transaction(command_name, True) except ServerSelectionTimeoutError: # Raise the original error so the application can infer that # an attempt was made. @@ -472,12 +471,30 @@ class ClientSession(object): if exc.code not in _RETRYABLE_ERROR_CODES: raise try: - return self._finish_transaction(command_name) + return self._finish_transaction(command_name, True) except ServerSelectionTimeoutError: # Raise the original error so the application can infer that # an attempt was made. raise exc + def _finish_transaction(self, command_name, retrying): + # Transaction spec says that after the initial commit attempt, + # subsequent commitTransaction commands should be upgraded to use + # w:"majority" and set a default value of 10 seconds for wtimeout. + wc = self._transaction.opts.write_concern + if retrying and command_name == "commitTransaction": + wc_doc = wc.document + wc_doc["w"] = "majority" + wc_doc.setdefault("wtimeout", 10000) + wc = WriteConcern(**wc_doc) + with self._client._socket_for_writes(self) as sock_info: + return self._client.admin._command( + sock_info, + command_name, + session=self, + write_concern=wc, + parse_write_concern_error=True) + def _advance_cluster_time(self, cluster_time): """Internal cluster time helper.""" if self._cluster_time is None: diff --git a/test/transactions/commit.json b/test/transactions/commit.json index 0a9331132..a348b41c7 100644 --- a/test/transactions/commit.json +++ b/test/transactions/commit.json @@ -315,7 +315,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -331,7 +334,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" diff --git a/test/transactions/error-labels.json b/test/transactions/error-labels.json index 52f7d0719..40d014ddb 100644 --- a/test/transactions/error-labels.json +++ b/test/transactions/error-labels.json @@ -746,7 +746,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -762,7 +765,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -878,7 +884,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -894,7 +903,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1023,7 +1035,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -1041,7 +1054,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -1171,7 +1185,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -1305,7 +1320,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", diff --git a/test/transactions/insert.json b/test/transactions/insert.json index 4dc9b85fc..34369bb92 100644 --- a/test/transactions/insert.json +++ b/test/transactions/insert.json @@ -233,6 +233,196 @@ } } }, + { + "description": "insert with session1", + "operations": [ + { + "name": "startTransaction", + "object": "session1" + }, + { + "name": "insertOne", + "object": "collection", + "arguments": { + "session": "session1", + "document": { + "_id": 1 + } + }, + "result": { + "insertedId": 1 + } + }, + { + "name": "insertMany", + "object": "collection", + "arguments": { + "documents": [ + { + "_id": 2 + }, + { + "_id": 3 + } + ], + "session": "session1" + }, + "result": { + "insertedIds": { + "0": 2, + "1": 3 + } + } + }, + { + "name": "commitTransaction", + "object": "session1" + }, + { + "name": "startTransaction", + "object": "session1" + }, + { + "name": "insertOne", + "object": "collection", + "arguments": { + "session": "session1", + "document": { + "_id": 4 + } + }, + "result": { + "insertedId": 4 + } + }, + { + "name": "abortTransaction", + "object": "session1" + } + ], + "expectations": [ + { + "command_started_event": { + "command": { + "insert": "test", + "documents": [ + { + "_id": 1 + } + ], + "ordered": true, + "readConcern": null, + "lsid": "session1", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": true, + "autocommit": false, + "writeConcern": null + }, + "command_name": "insert", + "database_name": "transaction-tests" + } + }, + { + "command_started_event": { + "command": { + "insert": "test", + "documents": [ + { + "_id": 2 + }, + { + "_id": 3 + } + ], + "ordered": true, + "lsid": "session1", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": null + }, + "command_name": "insert", + "database_name": "transaction-tests" + } + }, + { + "command_started_event": { + "command": { + "commitTransaction": 1, + "lsid": "session1", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": null + }, + "command_name": "commitTransaction", + "database_name": "admin" + } + }, + { + "command_started_event": { + "command": { + "insert": "test", + "documents": [ + { + "_id": 4 + } + ], + "ordered": true, + "readConcern": { + "afterClusterTime": 42 + }, + "lsid": "session1", + "txnNumber": { + "$numberLong": "2" + }, + "startTransaction": true, + "autocommit": false, + "writeConcern": null + }, + "command_name": "insert", + "database_name": "transaction-tests" + } + }, + { + "command_started_event": { + "command": { + "abortTransaction": 1, + "lsid": "session1", + "txnNumber": { + "$numberLong": "2" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": null + }, + "command_name": "abortTransaction", + "database_name": "admin" + } + } + ], + "outcome": { + "collection": { + "data": [ + { + "_id": 1 + }, + { + "_id": 2 + }, + { + "_id": 3 + } + ] + } + } + }, { "description": "collection writeConcern without transaction", "operations": [ diff --git a/test/transactions/retryable-abort.json b/test/transactions/retryable-abort.json index d2ab05dc9..33dd299e0 100644 --- a/test/transactions/retryable-abort.json +++ b/test/transactions/retryable-abort.json @@ -691,7 +691,7 @@ } }, { - "description": "abortTransaction succeeds after InterruptedDueToReplStateChange", + "description": "abortTransaction succeeds after InterruptedDueToStepDown", "failPoint": { "configureFailPoint": "failCommand", "mode": { @@ -1613,7 +1613,7 @@ } }, { - "description": "abortTransaction succeeds after WriteConcernError InterruptedDueToReplStateChange", + "description": "abortTransaction succeeds after WriteConcernError InterruptedDueToStepDown", "failPoint": { "configureFailPoint": "failCommand", "mode": { diff --git a/test/transactions/retryable-commit.json b/test/transactions/retryable-commit.json index a4a6850cb..30cacac01 100644 --- a/test/transactions/retryable-commit.json +++ b/test/transactions/retryable-commit.json @@ -105,7 +105,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -121,8 +124,167 @@ }, "startTransaction": null, "autocommit": false, + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } + }, + "command_name": "commitTransaction", + "database_name": "admin" + } + } + ], + "outcome": { + "collection": { + "data": [ + { + "_id": 1 + } + ] + } + } + }, + { + "description": "commitTransaction applies majority write concern on retries", + "clientOptions": { + "retryWrites": false + }, + "failPoint": { + "configureFailPoint": "failCommand", + "mode": { + "times": 2 + }, + "data": { + "failCommands": [ + "commitTransaction" + ], + "closeConnection": true + } + }, + "operations": [ + { + "name": "startTransaction", + "object": "session0", + "arguments": { + "options": { + "writeConcern": { + "w": 2, + "j": true, + "wtimeout": 5000 + } + } + } + }, + { + "name": "insertOne", + "object": "collection", + "arguments": { + "session": "session0", + "document": { + "_id": 1 + } + }, + "result": { + "insertedId": 1 + } + }, + { + "name": "commitTransaction", + "object": "session0", + "result": { + "errorLabelsContain": [ + "UnknownTransactionCommitResult" + ], + "errorLabelsOmit": [ + "TransientTransactionError" + ] + } + }, + { + "name": "commitTransaction", + "object": "session0" + } + ], + "expectations": [ + { + "command_started_event": { + "command": { + "insert": "test", + "documents": [ + { + "_id": 1 + } + ], + "ordered": true, + "readConcern": null, + "lsid": "session0", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": true, + "autocommit": false, "writeConcern": null }, + "command_name": "insert", + "database_name": "transaction-tests" + } + }, + { + "command_started_event": { + "command": { + "commitTransaction": 1, + "lsid": "session0", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": { + "w": 2, + "j": true, + "wtimeout": 5000 + } + }, + "command_name": "commitTransaction", + "database_name": "admin" + } + }, + { + "command_started_event": { + "command": { + "commitTransaction": 1, + "lsid": "session0", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": { + "w": "majority", + "j": true, + "wtimeout": 5000 + } + }, + "command_name": "commitTransaction", + "database_name": "admin" + } + }, + { + "command_started_event": { + "command": { + "commitTransaction": 1, + "lsid": "session0", + "txnNumber": { + "$numberLong": "1" + }, + "startTransaction": null, + "autocommit": false, + "writeConcern": { + "w": "majority", + "j": true, + "wtimeout": 5000 + } + }, "command_name": "commitTransaction", "database_name": "admin" } @@ -421,7 +583,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -526,7 +691,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -631,7 +799,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -736,7 +907,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -754,7 +928,7 @@ } }, { - "description": "commitTransaction succeeds after InterruptedDueToReplStateChange", + "description": "commitTransaction succeeds after InterruptedDueToStepDown", "failPoint": { "configureFailPoint": "failCommand", "mode": { @@ -841,7 +1015,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -946,7 +1123,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1051,7 +1231,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1156,7 +1339,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1261,7 +1447,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1366,7 +1555,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1471,7 +1663,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1576,7 +1771,10 @@ }, "startTransaction": null, "autocommit": false, - "writeConcern": null + "writeConcern": { + "w": "majority", + "wtimeout": 10000 + } }, "command_name": "commitTransaction", "database_name": "admin" @@ -1693,7 +1891,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -1712,7 +1911,7 @@ } }, { - "description": "commitTransaction succeeds after WriteConcernError InterruptedDueToReplStateChange", + "description": "commitTransaction succeeds after WriteConcernError InterruptedDueToStepDown", "failPoint": { "configureFailPoint": "failCommand", "mode": { @@ -1811,7 +2010,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -1929,7 +2129,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction", @@ -2047,7 +2248,8 @@ "startTransaction": null, "autocommit": false, "writeConcern": { - "w": "majority" + "w": "majority", + "wtimeout": 10000 } }, "command_name": "commitTransaction",