Skip to content

Commit

Permalink
Fix race condition in non-container transactions (#3671)
Browse files Browse the repository at this point in the history
* Transaction logic establishes 2 separate Promise Chains...

... One chain is for INTERNAL use within the Transaction logic
itself.  This chain ignores exceptions.  It is simply used to
signal when the "Next Transaction" (sibling) is allowed to begin.

The other chain is for EXTERNAL use.  It expects the caller
to handle any exceptions that occur during the Transaction.
  • Loading branch information
briandamaged committed Feb 19, 2020
1 parent 7288608 commit eaf0d15
Showing 1 changed file with 21 additions and 15 deletions.
36 changes: 21 additions & 15 deletions lib/transaction.js
Expand Up @@ -55,7 +55,11 @@ class Transaction extends EventEmitter {
outerTx ? 'nested' : 'top level'
);

this._promise = this.acquireConnection(config, (connection) => {
// FYI: As you will see in a moment, this Promise will be used to construct
// 2 separate Promise Chains. This ensures that each Promise Chain
// can establish its error-handling semantics without interfering
// with the other Promise Chain.
const basePromise = this.acquireConnection(config, (connection) => {
const trxClient = (this.trxClient = makeTxClient(
this,
client,
Expand All @@ -74,13 +78,7 @@ class Transaction extends EventEmitter {
return makeTransactor(this, connection, trxClient);
})
.then((transactor) => {
if (this.initPromise) {
transactor.executionPromise = executionPromise.catch((err) => {
throw err;
});
} else {
transactor.executionPromise = executionPromise;
}
transactor.executionPromise = executionPromise;

// If we've returned a "thenable" from the transaction container, assume
// the rollback and commit are chained to this object's success / failure.
Expand Down Expand Up @@ -115,6 +113,11 @@ class Transaction extends EventEmitter {
}
});


// FYI: This is the Promise Chain for EXTERNAL use. It ensures that the
// caller must handle any exceptions that result from `basePromise`.
this._promise = basePromise.then((x)=> x);

this._completed = false;

// If there's a wrapping transaction, we need to wait for any older sibling
Expand All @@ -124,7 +127,11 @@ class Transaction extends EventEmitter {
this._previousSibling = Bluebird.resolve(true);
if (outerTx) {
if (outerTx._lastChild) this._previousSibling = outerTx._lastChild;
outerTx._lastChild = this._promise;

// FYI: This is the Promise Chain for INTERNAL use. It serves as a signal
// for when the next sibling should begin its execution. Therefore,
// exceptions are caught and ignored.
outerTx._lastChild = basePromise.catch(()=> {});
}
}

Expand Down Expand Up @@ -225,12 +232,11 @@ class Transaction extends EventEmitter {
.then((connection) => {
connection.__knexTxId = this.txid;

return (this._previousSibling
? this._previousSibling.catch(() => {})
: Promise.resolve()
).then(function() {
return connection;
});
return this._previousSibling
// .catch(() => {}) // TODO: Investigate this line
.then(function() {
return connection;
});
})
.then(async (connection) => {
try {
Expand Down

0 comments on commit eaf0d15

Please sign in to comment.