The original code for handling nested transactions did not work correctly when you had sibling nested transactions, i.e. two or more transactions all living inside the same parent transaction and latter ones getting started after their predecessor sibling completes (rolls back or commits). In such cases, knex would hang on any DB operation attempted using a non-initial sibling transaction.
Basic cause was in knex accidentally waiting for that same transaction to complete before allowing someone else to access the transaction's database connection.
Basic bug fixed in d0836b8.
Detailed tests added for handling sibling tested transactions in 675c684. You can run those tests to reproduce the hang if you run them without the fix applied.
Hope this helps.
Before, if we had parent transaction A, and two nested sibling transactions inside it: B1 & B2, knex would hang when you asked it to execute any kind of a database operation in the second child transaction. Also, it had a bad Promise.settle() call that would break in case the previous child transaction promise not resolve to an array, which typically (e.g. when calling trx.commit() on the passed transaction object or resolving the transaction promise with no value) it would not.
- removed an unnecessary `||` with an always `undefined` value - avoided waiting for an always resolved promise
The only thing each transaction needs to track is its last direct child transaction. That is then used to prevent each sibling transaction from starting (i.e. returning its connection) before its predecessor transaction completed.
- improved related comments - renamed `trx._queue` to `trx._previousSibling` - made code waiting for `trx._previousSibling`, instead of code initializing that promise, more explicit about waiting for the promise to be either resolved or rejected, i.e. `settled`/`completed` - made the code a bit more compact
Note that this pull request does not attack the issue of why we'd want to try to synchronize sibling nested transactions in the first place, but only makes sure that the logic already implemented no longer breaks.
The current code, due to the way transactions perform their
@jurko-gospodnetic I'm not too familiar with the code for transactions, but the changes here makes sense to me and it seems you thoroughly tested this with several new tests and also updated the inline documentation so I am perfectly fine with merging this.