Propagate commit errors to AQL (#3831) (#3932)

arangod/Aql/Query.cpp

@@ -662,7 +662,10 @@ QueryResult Query::execute(QueryRegistry* registry) {
                                       << "Query::execute: before _trx->commit"
                                       << " this: " << (uintptr_t) this;
 
-    _trx->commit();
+    auto commitResult = _trx->commit();
+    if (commitResult.fail()) {
+        THROW_ARANGO_EXCEPTION(commitResult);
+    }
     
     LOG_TOPIC(DEBUG, Logger::QUERIES)
         << TRI_microtime() - _startTime << " "
@@ -864,7 +867,10 @@ QueryResultV8 Query::executeV8(v8::Isolate* isolate, QueryRegistry* registry) {
                                       << "Query::executeV8: before _trx->commit"
                                       << " this: " << (uintptr_t) this;
 
-    _trx->commit();
+    auto commitResult = _trx->commit();
+    if (commitResult.fail()) {
+        THROW_ARANGO_EXCEPTION(commitResult);
+    }
 
     LOG_TOPIC(DEBUG, Logger::QUERIES)
         << TRI_microtime() - _startTime << " "
@@ -1022,9 +1028,12 @@ QueryResult Query::explain() {
                        !_isModificationQuery && _warnings.empty() &&
                        _ast->root()->isCacheable());
     }
-
-    _trx->commit();
-
+    
+    auto commitResult = _trx->commit();
+    if (commitResult.fail()) {
+        THROW_ARANGO_EXCEPTION(commitResult);
+    }
+    
     result.warnings = warningsToVelocyPack();
 
     result.stats = opt._stats.toVelocyPack();

arangod/Transaction/Methods.cpp

@@ -710,7 +710,7 @@ Result transaction::Methods::begin() {
     if (_state->isTopLevelTransaction()) {
       _state->updateStatus(transaction::Status::RUNNING);
     }
-    return TRI_ERROR_NO_ERROR;
+    return Result();
   }
 
   return _state->beginTransaction(_localHints);
@@ -718,9 +718,13 @@ Result transaction::Methods::begin() {
 
 /// @brief commit / finish the transaction
 Result transaction::Methods::commit() {
+  TRI_IF_FAILURE("TransactionCommitFail") {
+    return Result(TRI_ERROR_DEBUG);
+  }
+
   if (_state == nullptr || _state->status() != transaction::Status::RUNNING) {
     // transaction not created or not running
-    return TRI_ERROR_TRANSACTION_INTERNAL;
+    return Result(TRI_ERROR_TRANSACTION_INTERNAL);
   }
   
   ExecContext const* exe = ExecContext::CURRENT;
@@ -737,7 +741,7 @@ Result transaction::Methods::commit() {
     if (_state->isTopLevelTransaction()) {
       _state->updateStatus(transaction::Status::COMMITTED);
     }
-    return TRI_ERROR_NO_ERROR;
+    return Result();
   }
 
   return _state->commitTransaction(this);
@@ -757,7 +761,7 @@ Result transaction::Methods::abort() {
       _state->updateStatus(transaction::Status::ABORTED);
     }
 
-    return TRI_ERROR_NO_ERROR;
+    return Result();
   }
 
   return _state->abortTransaction(this);

js/server/tests/shell/shell-transactions-noncluster.js

@@ -64,6 +64,118 @@ var sortedKeys = function (col) {
   return keys;
 };
 
+function transactionFailuresSuite () {
+  'use strict';
+  var cn = 'UnitTestsTransaction';
+  var c = null;
+
+  return {
+
+    setUp: function () {
+      internal.debugClearFailAt();
+      db._drop(cn);
+      c = db._create(cn);
+    },
+
+    tearDown: function () {
+      internal.debugClearFailAt();
+
+      if (c !== null) {
+        c.drop();
+      }
+
+      c = null;
+      internal.wait(0);
+    },
+    
+    testCommitEmptyTransactionFailure : function () {
+      c.insert({ _key: "foobar", value: "baz" });
+      assertEqual(1, c.count());
+
+      internal.debugSetFailAt("TransactionCommitFail");
+      try {
+        db._executeTransaction({ 
+          collections: {
+            write: cn 
+          },
+          action: function () {}
+        });
+
+        fail();
+      } catch (err) {
+        assertEqual(internal.errors.ERROR_DEBUG.code, err.errorNum);
+      }
+
+      internal.debugClearFailAt();
+      assertEqual(1, c.count());
+      assertEqual("baz", c.document("foobar").value);
+    },
+
+    testCommitTransactionWithRemovalsFailure : function () {
+      for (var i = 0; i < 100; ++i) {
+        c.insert({ _key: "test" + i });
+      }
+      assertEqual(100, c.count());
+      
+      internal.debugSetFailAt("TransactionCommitFail");
+      try {
+        db._executeTransaction({ 
+          collections: {
+            write: cn 
+          },
+          action: function () {
+            for (var i = 0; i < 100; ++i) {
+              c.remove("test" + i);
+            }
+            assertEqual(0, c.count());
+          }
+        });
+
+        fail();
+      } catch (err) {
+        assertEqual(internal.errors.ERROR_DEBUG.code, err.errorNum);
+      }
+
+      internal.debugClearFailAt();
+      assertEqual(100, c.count());
+    },
+    
+    testCommitTransactionWithFailuresInsideFailure : function () {
+      c.insert({ _key: "foobar", value: "baz" });
+
+      internal.debugSetFailAt("TransactionCommitFail");
+      try {
+        db._executeTransaction({ 
+          collections: {
+            write: cn 
+          },
+          action: function () {
+            for (var i = 0; i < 100; ++i) {
+              try {
+                // insert conflicting document
+                c.insert({ _key: "foobar" });
+                fail();
+              } catch (err) {
+              }
+            }
+
+            assertEqual(1, c.count());
+          }
+        });
+
+        fail();
+      } catch (err) {
+        assertEqual(internal.errors.ERROR_DEBUG.code, err.errorNum);
+      }
+
+      internal.debugClearFailAt();
+      assertEqual(1, c.count());
+      assertEqual("baz", c.document("foobar").value);
+    }
+
+  };
+}
+
 function transactionRevisionsSuite () {
   'use strict';
   var cn = 'UnitTestsTransaction';
@@ -3918,6 +4030,9 @@ function transactionTraversalSuite () {
 // / @brief executes the test suites
 // //////////////////////////////////////////////////////////////////////////////
 
+if (internal.debugCanUseFailAt()) {
+  jsunity.run(transactionFailuresSuite);
+}
 jsunity.run(transactionRevisionsSuite);
 jsunity.run(transactionRollbackSuite);
 jsunity.run(transactionInvocationSuite);