Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Fix race condition in non-container transactions #3671

Merged
merged 3 commits into from Feb 19, 2020
Merged

Fix race condition in non-container transactions #3671

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
393ab30
rm unnecessary conditional (this._previousSibling always defined)
briandamaged Feb 19, 2020
ef471e1
rm catch/rethrow logic for transactor.executionPromise ...
briandamaged Feb 19, 2020
4a58ae2
Transaction logic establishes 2 separate Promise Chains...
briandamaged Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 6 additions & 13 deletions lib/transaction.js
View file
Open in desktop
Expand Up @@ -74,13 +74,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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximelkin : I'm fairly certain that this change is the one that fixed the race condition.

Your PR (#3665) included this change as well; however, it relocated the .catch((err)=> {throw err}) logic to a different part of the code. In contrast, this PR just removes that logic entirely.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch added in #3328, so I am unsure about this changes


// 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 @@ -225,12 +219,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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acquireConnection also has overrides in several dialects, check them please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Transaction's acquireConnection method delegates to the underlying Dialect's acquireConnection method:

knex/lib/transaction.js

Lines 216 to 224 in 7288608

acquireConnection(config, cb) {
const configConnection = config && config.connection;
return new Bluebird((resolve, reject) => {
try {
resolve(configConnection || this.client.acquireConnection());
} catch (e) {
reject(e);
}
})

So, there isn't actually an inheritance relationship there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, nvm. I see what you mean. I'll check it in a little bit.

(Related note: seems like there is a lot of duplication within those implementations)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the other implementations are actually ignoring the details around this._previousSibling. That is potentially a problem, but not a problem that was introduced by this specific change.

.catch(() => {}) // TODO: Investigate this line
.then(function() {
return connection;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was just a minor code cleanup. It turns out that this._previousSibling is always defined due to the way the constructor is written. So, there was no need for conditional logic here.

Additionally, take note of the TODO. I'm 95% sure that the code should not be ignoring that exception. However, I left it in place for now so that we can minimize the impact of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... I suspect that the .catch(()=> {}) line was the reason for the "silent failures" mentioned here:

#3328 (comment)

I'm going to test out a different implementation that will (hopefully!) allow for proper sibling->sibling linking without swallowing exceptions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright -- I just pushed the code for establishing 2 separate Promise Chains. So, instead of doing this:

this._promise = createPromise()
outer._lastChild = this._promise.catch(()=> {})
// Oops!  Now `this.promise` will always think that its exceptions have been handled!

It is doing this:

const basePromise = createPromise();
this._promise = basePromise.then((x)=> x)
outer._lastChild = this._promise.catch(()=> {})
// Yay!  Now `this._promise` understands that its exceptions still need to be handled!

})
.then(async (connection) => {
try {
Expand Down