oracledb: commit was a no-op causing race conditions (#3668)

The underlying issue was that query *always* committed, even during a transaction. The previous fix was to just disable commitAsync in a transaction but then that also disabled explicit commit calls. Instead, this fix disables query's commits during transactions so that explicit commits still work.

lib/dialects/oracledb/index.js

@@ -108,9 +108,6 @@ Client_Oracledb.prototype.acquireRawConnection = function() {
       }
       connection.commitAsync = function() {
         return new Bluebird((commitResolve, commitReject) => {
-          if (connection.isTransaction) {
-            return commitResolve();
-          }
           this.commit(function(err) {
             if (err) {
               return commitReject(err);
@@ -293,7 +290,9 @@ Client_Oracledb.prototype._query = function(connection, obj) {
     }
 
     if (!obj.returning && outBinds.length === 0) {
-      await connection.commitAsync();
+      if (!connection.isTransaction) {
+        await connection.commitAsync();
+      }
       return obj;
     }
     const rowIds = [];
@@ -335,17 +334,15 @@ Client_Oracledb.prototype._query = function(connection, obj) {
         });
       }
     }
-    return connection.commitAsync().then(function() {
-      if (obj.returningSql) {
-        return connection
-          .executeAsync(obj.returningSql(), rowIds, { resultSet: true })
-          .then(function(response) {
-            obj.response = response.rows;
-            return obj;
-          });
-      }
+    if (connection.isTransaction) {
       return obj;
-    });
+    }
+    await connection.commitAsync();
+    if (obj.returningSql) {
+      const response = await connection.executeAsync(obj.returningSql(), rowIds, { resultSet: true })
+      obj.response = response.rows;
+    }
+    return obj;
   });
 };
 

lib/dialects/oracledb/transaction.js

@@ -11,12 +11,14 @@ module.exports = class Oracle_Transaction extends Transaction {
     return Bluebird.resolve();
   }
 
-  commit(conn, value) {
+  async commit(conn, value) {
     this._completed = true;
-    return conn
-      .commitAsync()
-      .then(() => value)
-      .then(this._resolver, this._rejecter);
+    try {
+      await conn.commitAsync();
+      this._resolver(value);
+    } catch (err) {
+      this._rejecter(err);
+    }
   }
 
   release(conn, value) {

test/integration/builder/inserts.js

@@ -1098,11 +1098,7 @@ module.exports = function(knex) {
         });
     });
 
-    describe('batchInsert (TODO: fix random oracle fail)', function() {
-      if (knex.client.driverName == 'oracledb') {
-        return;
-      }
-
+    describe('batchInsert', function() {
       const driverName = knex.client.driverName;
       const fiftyLengthString =
         'rO8F8YrFS6uoivuRiVnwrO8F8YrFS6uoivuRiVnwuoivuRiVnw';

test/integration/builder/transaction.js

@@ -79,12 +79,7 @@ module.exports = function(knex) {
         });
     });
 
-    it('should be able to commit transactions (TODO: fix random oracle fail)', function() {
-      if (knex.client.driverName == 'oracledb') {
-        this.skip();
-        return;
-      }
-
+    it('should be able to commit transactions', function() {
       let id = null;
       return knex
         .transaction(function(t) {
@@ -167,12 +162,7 @@ module.exports = function(knex) {
         });
     });
 
-    it('should be able to commit transactions with a resolved trx query (TODO: fix random oracle fail)', function() {
-      if (knex.client.driverName == 'oracledb') {
-        this.skip();
-        return;
-      }
-
+    it('should be able to commit transactions with a resolved trx query', function() {
       let id = null;
       return knex
         .transaction(function(trx) {

test/integration/builder/updates.js

@@ -70,18 +70,7 @@ module.exports = function(knex) {
         });
     });
 
-    it('should immediately return updated value for other connections when updating row to DB returns (TODO: fix oracle fail)', function() {
-      if (knex.client.driverName == 'oracledb') {
-        // TODO: this test was added to catch strange random fails with oracle
-        // currently it looks like at least oracle transactions seem to return before
-        // they are actually committed to database...
-
-        // if those selects are changed to forUpdate() then the test seem to pass fine
-
-        this.skip();
-        return;
-      }
-
+    it('should immediately return updated value for other connections when updating row to DB returns', function() {
       return knex('accounts').then((res) => {
         function runTest() {
           return Promise.all(

test/integration/migrate/index.js

@@ -245,12 +245,7 @@ module.exports = function(knex) {
         });
       });
 
-      it('should remove the record in the lock table once finished (TODO: fix random oracle fail)', function() {
-        if (knex.client.driverName == 'oracledb') {
-          this.skip();
-          return;
-        }
-
+      it('should remove the record in the lock table once finished', function() {
         return knex('knex_migrations_lock')
           .select('*')
           .then(function(data) {

test/integration/suite.js

@@ -46,11 +46,7 @@ module.exports = function(knex) {
   });
 
   describe('knex.initialize', function() {
-    it('should allow initialize the pool with knex.initialize (TODO: fix oracle)', function() {
-      if (knex.client.driverName === 'oracledb') {
-        this.skip();
-        return;
-      }
+    it('should allow initialize the pool with knex.initialize', function() {
       expect(knex.client.pool).to.equal(undefined);
       knex.initialize();
       expect(knex.client.pool.destroyed).to.equal(false);