migrate: Refactor _lockMigrations to avoid forUpdate (#3395)

lib/migrate/Migrator.js

@@ -259,32 +259,23 @@ class Migrator {
     }
   }
 
-  _isLocked(trx) {
-    const tableName = getLockTableName(this.config.tableName);
-    return getTable(this.knex, tableName, this.config.schemaName)
-      .transacting(trx)
-      .forUpdate()
-      .select('*')
-      .then((data) => data[0].is_locked);
-  }
-
   _lockMigrations(trx) {
     const tableName = getLockTableName(this.config.tableName);
     return getTable(this.knex, tableName, this.config.schemaName)
       .transacting(trx)
-      .update({ is_locked: 1 });
+      .where('is_locked', '=', 0)
+      .update({ is_locked: 1 })
+      .then((rowCount) => {
+        if (rowCount != 1) {
+          throw new Error('Migration table is already locked');
+        }
+      });
   }
 
   _getLock(trx) {
     const transact = trx ? (fn) => fn(trx) : (fn) => this.knex.transaction(fn);
     return transact((trx) => {
-      return this._isLocked(trx)
-        .then((isLocked) => {
-          if (isLocked) {
-            throw new Error('Migration table is already locked');
-          }
-        })
-        .then(() => this._lockMigrations(trx));
+      return this._lockMigrations(trx);
     }).catch((err) => {
       throw new LockError(err.message);
     });

test/integration/migrate/index.js

@@ -261,6 +261,57 @@ module.exports = function(knex) {
           });
       });
 
+      it('should work with concurent calls to _lockMigrations', async function() {
+        if (knex.client.driverName == 'sqlite3') {
+          // sqlite doesn't support concurrency
+          this.skip();
+          return;
+        }
+
+        const migrator = knex.migrate;
+        try {
+          // Start two transactions and call _lockMigrations in each of them.
+          // Simulate a race condition by waiting until both are started before
+          // attempting to commit either one. Exactly one should succeed.
+          //
+          // Both orderings are legitimate, but in practice the first transaction
+          // to start will be the one that succeeds in all currently supported
+          // databases (CockroachDB 1.x is an example of a database where the
+          // second transaction would win, but this changed in 2.0). This test
+          // assumes the first transaction wins, but could be refactored to support
+          // both orderings if desired.
+          const trx1 = await knex.transaction();
+          await migrator._lockMigrations(trx1);
+          const trx2 = await knex.transaction();
+          // trx1 has a pending write lock, so the second call to _lockMigrations
+          // will block (unless we're on a DB that resolves the transaction in
+          // the other order as mentioned above).
+          // Save the promise, then wait a short time to ensure it's had time
+          // to start its query and get blocked.
+          const trx2Promise = migrator._lockMigrations(trx2);
+          await Bluebird.delay(100);
+          if (!trx2Promise.isPending()) {
+            throw new Error('expected trx2 to be pending');
+          }
+          await trx1.commit();
+          // trx1 has completed and unblocked trx2, which should now fail.
+          try {
+            await trx2Promise;
+            throw new Error('expected trx2 to fail');
+          } catch (error) {
+            expect(error)
+              .to.have.property('message')
+              .that.includes('already locked');
+            await trx2.rollback();
+          }
+        } finally {
+          // Clean up after ourselves (I'm not sure why the before() at the
+          // top of this file isn't doing it, but if this test fails without
+          // this call it tends to cause cascading failures).
+          await migrator._freeLock();
+        }
+      });
+
       it('should report failing migration', function() {
         const migrator = knex.migrate;
         return migrator