New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed nested transactions #1226

Merged
merged 5 commits into from Mar 7, 2016

Conversation

Projects
None yet
3 participants
@jurko-gospodnetic
Collaborator

jurko-gospodnetic commented Feb 25, 2016

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.

The three later commits (e0e77df, f38f901, 675c684) then just clean up the related code. And in the end the code is simplified enough that the original fix becomes unnecessary to begin with. 馃槃

Hope this helps.

Best regards,
Jurko Gospodneti膰

jurko-gospodnetic added some commits Feb 24, 2016

fix synchronizing sibling nested transactions
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.
simplify nested transaction _.queue variable initialization
- removed an unnecessary `||` with an always `undefined` value
- avoided waiting for an always resolved promise
simplify nested transaction - no need to track _childQueue
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.
clean up sibling nested transaction tracking code
- 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
@jurko-gospodnetic

This comment has been minimized.

Collaborator

jurko-gospodnetic commented Feb 26, 2016

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 SAVEPOINT commands directly at transaction startup, already depends on callers keeping such sibling nested transactions in sync by properly chaining their promises. Thus, it might be that this internal synchronization is either not needed, or needs to be fixed to work for cases when the caller does not chain those promises correctly. But I think that's a topic for a separate story. As there are no tests related to that, I was loath to do that cleanup here lest I confuse the original hang issue that needed to be fixed.

@jurko-gospodnetic

This comment has been minimized.

Collaborator

jurko-gospodnetic commented Mar 7, 2016

Ping?

@wubzz & @rhys-vdw - you guys appeared to take a look at my last pull request, not sure who's the correct person to ping about this one or who gets notifications about newly added pull requests here.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Mar 7, 2016

@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.

Thanks!

wubzz added a commit that referenced this pull request Mar 7, 2016

@wubzz wubzz merged commit a5ece5e into tgriesser:master Mar 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jurko-gospodnetic jurko-gospodnetic deleted the jurko-gospodnetic:fix-nested-transactions branch Mar 7, 2016

@@ -238,7 +222,7 @@ function makeTxClient(trx, client, connection) {
})
}
trxClient.acquireConnection = function() {
return trx._queue.then(function() {
return Promise.settle([trx._previousSibling]).then(function () {

This comment has been minimized.

@elhigu

elhigu Mar 14, 2016

Collaborator

Is it ok here to continue if _previousSibling did fail? Edit: looks like previous implementation did use settle too...

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic Mar 14, 2016

Collaborator

This behaviour is unchanged. And from how I read the original code, the idea is that if we have a set of transactions like this:

TP (parent)
    - TS1 (first sibling)
    - TS2 (second sibling)

created using code like this:

knex.transaction(function (tp) {
    tp.transaction(function (ts1) {
        ...  // returns a promise indicating when ts1 completes
    });
    tp.transaction(function (ts2) {
        ...  // returns a promise indicating when ts2 completes
    });
});

TS2 should be allowed to proceed (i.e. acquire its connection so it can perform actual database operations) only after TS1 has completed its work. Whether TS1 got committed or rolled back.

If we want TS2 to be run only if TS1 gets committed, then we should create TS2 in a .then() block chained to the TS1 resolution promise, and not just create it as a completely unrelated sibling transaction. i.e. they should created using code something like this:

knex.transaction(function (tp) {
    tp.transaction(function (ts1) {
        ...  // returns a promise indicating when ts1 completes
    })
    .then(function () {
        return tp.transaction(function (ts2) {
            ...  // returns a promise indicating when ts2 completes
        });
    }
});
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment