fix(mysql/mariadb): treat deadlocked transactions as rollback (#11074)

schmod · sushantdhiman · commit 003aabcc812f · 2019-07-11T09:22:09.000+05:30
diff --git a/lib/dialects/mariadb/query.js b/lib/dialects/mariadb/query.js
@@ -34,10 +34,10 @@ class Query extends AbstractQuery {
 
   run(sql, parameters) {
     this.sql = sql;
-    const { connection } = this;
+    const { connection, options } = this;
 
     const showWarnings = this.sequelize.options.showWarnings
-      || this.options.showWarnings;
+      || options.showWarnings;
 
     const complete = this._logQuery(sql, debug);
 
@@ -56,6 +56,11 @@ class Query extends AbstractQuery {
           return results;
         })
         .catch(err => {
+          // MariaDB automatically rolls-back transactions in the event of a deadlock
+          if (options.transaction && err.errno === 1213) {
+            options.transaction.finished = 'rollback';
+          }
+
           complete();
 
           err.sql = sql;
diff --git a/lib/dialects/mysql/query.js b/lib/dialects/mysql/query.js
@@ -29,10 +29,10 @@ class Query extends AbstractQuery {
 
   run(sql, parameters) {
     this.sql = sql;
-    const { connection } = this;
+    const { connection, options } = this;
 
     //do we need benchmark for this query execution
-    const showWarnings = this.sequelize.options.showWarnings || this.options.showWarnings;
+    const showWarnings = this.sequelize.options.showWarnings || options.showWarnings;
 
     const complete = this._logQuery(sql, debug);
 
@@ -41,6 +41,10 @@ class Query extends AbstractQuery {
         complete();
 
         if (err) {
+          // MySQL automatically rolls-back transactions in the event of a deadlock
+          if (options.transaction && err.errno === 1213) {
+            options.transaction.finished = 'rollback';
+          }
           err.sql = sql;
 
           reject(this.formatError(err));
diff --git a/lib/query-interface.js b/lib/query-interface.js
@@ -1417,7 +1417,8 @@ class QueryInterface {
 
     options = Object.assign({}, options, {
       transaction: transaction.parent || transaction,
-      supportsSearchPath: false
+      supportsSearchPath: false,
+      completesTransaction: true
     });
 
     const sql = this.QueryGenerator.commitTransactionQuery(transaction);
@@ -1435,7 +1436,8 @@ class QueryInterface {
 
     options = Object.assign({}, options, {
       transaction: transaction.parent || transaction,
-      supportsSearchPath: false
+      supportsSearchPath: false,
+      completesTransaction: true
     });
     options.transaction.name = transaction.parent ? transaction.name : undefined;
     const sql = this.QueryGenerator.rollbackTransactionQuery(transaction);
diff --git a/lib/sequelize.js b/lib/sequelize.js
@@ -614,24 +614,30 @@ class Sequelize {
         [sql, bindParameters] = this.dialect.Query.formatBindParameters(sql, options.bind, this.options.dialect);
       }
 
+      const checkTransaction = () => {
+        if (options.transaction && options.transaction.finished && !options.completesTransaction) {
+          const error = new Error(`${options.transaction.finished} has been called on this transaction(${options.transaction.id}), you can no longer use it. (The rejected query is attached as the 'sql' property of this error)`);
+          error.sql = sql;
+          throw error;
+        }
+      };
+
       const retryOptions = Object.assign({}, this.options.retry, options.retry || {});
 
       return Promise.resolve(retry(() => Promise.try(() => {
         if (options.transaction === undefined && Sequelize._cls) {
           options.transaction = Sequelize._cls.get('transaction');
         }
-        if (options.transaction && options.transaction.finished) {
-          const error = new Error(`${options.transaction.finished} has been called on this transaction(${options.transaction.id}), you can no longer use it. (The rejected query is attached as the 'sql' property of this error)`);
-          error.sql = sql;
-          throw error;
-        }
+
+        checkTransaction();
 
         return options.transaction
           ? options.transaction.connection
           : this.connectionManager.getConnection(options);
       }).then(connection => {
         const query = new this.dialect.Query(connection, this, options);
         return this.runHooks('beforeQuery', options, query)
+          .then(() => checkTransaction())
           .then(() => query.run(sql, bindParameters))
           .finally(() => this.runHooks('afterQuery', options, query))
           .finally(() => {
diff --git a/test/integration/dialects/mysql/errors.test.js b/test/integration/dialects/mysql/errors.test.js
@@ -70,7 +70,6 @@ if (dialect === 'mysql') {
             reltype: 'child'
           }));
       });
-
     });
   });
 }
diff --git a/test/integration/transaction.test.js b/test/integration/transaction.test.js
@@ -366,6 +366,68 @@ if (current.dialect.supports.transactions) {
       });
     });
 
+    if (dialect === 'mysql' || dialect === 'mariadb') {
+      describe('deadlock handling', () => {
+        it('should treat deadlocked transaction as rollback', function() {
+          const Task = this.sequelize.define('task', {
+            id: {
+              type: Sequelize.INTEGER,
+              primaryKey: true
+            }
+          });
+
+          // This gets called twice simultaneously, and we expect at least one of the calls to encounter a
+          // deadlock (which effectively rolls back the active transaction).
+          // We only expect createTask() to insert rows if a transaction is active.  If deadlocks are handled
+          // properly, it should only execute a query if we're actually inside a real transaction.  If it does
+          // execute a query, we expect the newly-created rows to be destroyed when we forcibly rollback by
+          // throwing an error.
+          // tl;dr; This test is designed to ensure that this function never inserts and commits a new row.
+          const update = (from, to) => this.sequelize.transaction(transaction => {
+            return Task.findAll({
+              where: {
+                id: {
+                  [Sequelize.Op.eq]: from
+                }
+              },
+              lock: 'UPDATE',
+              transaction
+            })
+              .then(() => Promise.delay(10))
+              .then(() => {
+                return Task.update({ id: to }, {
+                  where: {
+                    id: {
+                      [Sequelize.Op.ne]: to
+                    }
+                  },
+                  lock: transaction.LOCK.UPDATE,
+                  transaction
+                });
+              })
+              .catch(e => { console.log(e.message); })
+              .then(() => Task.create({ id: 2 }, { transaction }))
+              .catch(e => { console.log(e.message); })
+              .then(() => { throw new Error('Rollback!'); });
+          }).catch(() => {});
+
+          return this.sequelize.sync({ force: true })
+            .then(() => Task.create({ id: 0 }))
+            .then(() => Task.create({ id: 1 }))
+            .then(() => Promise.all([
+              update(1, 0),
+              update(0, 1)
+            ]))
+            .then(() => {
+              return Task.count().then(count => {
+                // If we were actually inside a transaction when we called `Task.create({ id: 2 })`, no new rows should be added.
+                expect(count).to.equal(2, 'transactions were fully rolled-back, and no new rows were added');
+              });
+            });
+        });
+      });
+    }
+
     if (dialect === 'sqlite') {
       it('provides persistent transactions', () => {
         const sequelize = new Support.Sequelize('database', 'username', 'password', { dialect: 'sqlite' }),

Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,6 @@ if (dialect === 'mysql') {`
`70`	`70`	`reltype: 'child'`
`71`	`71`	`}));`
`72`	`72`	`});`
`73`		`-`
`74`	`73`	`});`
`75`	`74`	`});`
`76`	`75`	`}`