From d50d00644c88378cd810f3eff9070fcc0b3e4b8c Mon Sep 17 00:00:00 2001 From: Jake Coffman Date: Sun, 16 Feb 2020 13:26:13 -0600 Subject: [PATCH] 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 | 25 +++++++++++-------------- lib/dialects/oracledb/transaction.js | 12 +++++++----- test/integration/builder/inserts.js | 6 +----- test/integration/builder/transaction.js | 14 ++------------ test/integration/builder/updates.js | 13 +------------ test/integration/migrate/index.js | 7 +------ test/integration/suite.js | 6 +----- 7 files changed, 24 insertions(+), 59 deletions(-) diff --git a/lib/dialects/oracledb/index.js b/lib/dialects/oracledb/index.js index 20bf7c4516..f7f7fcb3b7 100644 --- a/lib/dialects/oracledb/index.js +++ b/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; }); }; diff --git a/lib/dialects/oracledb/transaction.js b/lib/dialects/oracledb/transaction.js index a759470032..6d6828a309 100644 --- a/lib/dialects/oracledb/transaction.js +++ b/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) { diff --git a/test/integration/builder/inserts.js b/test/integration/builder/inserts.js index 3f72a506a4..a95c5892ff 100644 --- a/test/integration/builder/inserts.js +++ b/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'; diff --git a/test/integration/builder/transaction.js b/test/integration/builder/transaction.js index e82ad9d760..c5bae4ef02 100644 --- a/test/integration/builder/transaction.js +++ b/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) { diff --git a/test/integration/builder/updates.js b/test/integration/builder/updates.js index a7245f659d..006361e536 100644 --- a/test/integration/builder/updates.js +++ b/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( diff --git a/test/integration/migrate/index.js b/test/integration/migrate/index.js index 5d00df84c5..496fbebdbb 100644 --- a/test/integration/migrate/index.js +++ b/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) { diff --git a/test/integration/suite.js b/test/integration/suite.js index 18ddd35e59..17d73710e5 100644 --- a/test/integration/suite.js +++ b/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);