Merge pull request #5243 from ajfranzoia/fix-transaction-finished-race-condition

Fix #5222: prevent race condition after transaction finished

changelog.md

 # Future
+- [FIXED] Prevent race condition after transaction finished [#5222](https://github.com/sequelize/sequelize/issues/5222)
+
+# Future
 - [FIXED] Fixed Instance.reload issues ([#4844](https://github.com/sequelize/sequelize/issues/4844) and [#4452](https://github.com/sequelize/sequelize/issues/4452))
 
 # 3.18.0

lib/query-interface.js

@@ -902,8 +902,11 @@ QueryInterface.prototype.commitTransaction = function(transaction, options) {
   });
 
   var sql = this.QueryGenerator.commitTransactionQuery(transaction);
+  var promise = this.sequelize.query(sql, options);
 
-  return this.sequelize.query(sql, options);
+  transaction.finished = 'commit';
+
+  return promise;
 };
 
 QueryInterface.prototype.rollbackTransaction = function(transaction, options) {
@@ -917,8 +920,11 @@ QueryInterface.prototype.rollbackTransaction = function(transaction, options) {
   });
 
   var sql = this.QueryGenerator.rollbackTransactionQuery(transaction);
+  var promise = this.sequelize.query(sql, options);
 
-  return this.sequelize.query(sql, options);
+  transaction.finished = 'rollback';
+
+  return promise;
 };
 
 module.exports = QueryInterface;

lib/transaction.js

@@ -211,11 +211,10 @@ Transaction.prototype.rollback = function() {
     .getQueryInterface()
     .rollbackTransaction(this, this.options)
     .finally(function() {
-      self.finished = 'rollback';
       if (!self.parent) {
         return self.cleanup();
       }
-      return null;
+      return self;
     });
 };
 

test/integration/transaction.test.js

@@ -129,6 +129,20 @@ describe(Support.getTestDialectTeaser('Transaction'), function() {
     ).to.eventually.be.rejected;
   });
 
+  it('does not allow queries immediatly after commit call', function() {
+    var self = this;
+    return expect(
+      this.sequelize.transaction().then(function(t) {
+        return self.sequelize.query('SELECT 1+1', {transaction: t, raw: true}).then(function() {
+          return Promise.join(
+            expect(t.commit()).to.eventually.be.fulfilled,
+            expect(self.sequelize.query('SELECT 1+1', {transaction: t, raw: true})).to.eventually.be.rejectedWith(/commit has been called on this transaction\([^)]+\), you can no longer use it/)
+          );
+        });
+      })
+    ).to.be.eventually.fulfilled;
+  });
+
   it('does not allow queries after rollback', function() {
     var self = this;
     return expect(
@@ -142,13 +156,27 @@ describe(Support.getTestDialectTeaser('Transaction'), function() {
     ).to.eventually.be.rejected;
   });
 
+  it('does not allow queries immediatly after rollback call', function() {
+    var self = this;
+    return expect(
+      this.sequelize.transaction().then(function(t) {
+        return Promise.join(
+          expect(t.rollback()).to.eventually.be.fulfilled,
+          expect(self.sequelize.query('SELECT 1+1', {transaction: t, raw: true})).to.eventually.be.rejectedWith(/rollback has been called on this transaction\([^)]+\), you can no longer use it/)
+        );
+      })
+    ).to.eventually.be.fulfilled;
+  });
+
   it('does not allow commits after commit', function () {
     var self = this;
-    return expect(self.sequelize.transaction().then(function (t) {
+    return expect(
+      self.sequelize.transaction().then(function (t) {
         return t.commit().then(function () {
           return t.commit();
         });
-    })).to.be.rejectedWith('Error: Transaction cannot be committed because it has been finished with state: commit');
+      })
+    ).to.be.rejectedWith('Error: Transaction cannot be committed because it has been finished with state: commit');
   });
 
   it('does not allow commits after rollback', function () {