-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cancelling timeouted queued queries in transactions for MySQL #4416
base: master
Are you sure you want to change the base?
Changes from 2 commits
6fb24fc
d860f11
53fabf1
b8dbd32
8c89de4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
// MySQL2 Client | ||
// ------- | ||
const findIndex = require('lodash/findIndex'); | ||
const Client_MySQL = require('../mysql'); | ||
const Transaction = require('./transaction'); | ||
|
||
|
@@ -20,6 +21,18 @@ class Client_MySQL2 extends Client_MySQL { | |
} | ||
return true; | ||
} | ||
|
||
_cancelQueuedQuery(connectionToKill, queryToKill) { | ||
const index = findIndex( | ||
connectionToKill._commands.toArray(), | ||
(cmd) => cmd.__knexQueryUid === queryToKill.__knexQueryUid | ||
); | ||
if (index >= 0) { | ||
connectionToKill._commands.removeOne(index); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is quite an invasion into the driver internals. Perhaps we could eventually ask the driver upstream to implement canceling of queued queries. But for now, there is no other way how to remove a query from the queue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Dont really like this idea. Can we implement it in drivers first? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, implement this probably bad idea, can we try to make things same for pg and mysql instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I can definitely try to make a PR to drivers. But first we should be sure this solution is acceptable for us.
Can you please explain what you mean by making things the same for pg and mysql? How would we make things the same for them? In MySQL there are two commands:
In Postgres there are also two commands:
We probably don't want to use So we have to use Also IMHO it is good to keep the connection open, so the user may still handle and resurrect the transation if one of their queries times out. In postgres they may want to roll back to some savepoint they created before running the slow query, in MySQL they may want to just catch the timeout exception and continue with another query. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest adding connection.removeQuery() to mysql/mysql2, which accepts driver Query, and which responses with one of three states: not executed (and cancelled), currently executing (no actions, we should cancel it), not found (probably executed or cancelled by some other way). currently executing - this case is dangerous, because cancel initialization could take too many time, so next query will be cancelled instead. Maybe we need lock mechanism to prevent sending any commands to mysql before query die. Also todo: check concurrent manual cancel + timeout cancel. EDIT: proposed solution not final, see comments in drivers issues There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maximelkin @kibertoad I filed the feature request to both drivers: Please provide every possible endorsment for this feature request in the drives on behalf of Knex. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @maximelkin @kibertoad I have created draft implementations of
Also I have adapted this PR to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I managed to simulate this race condition in a test. And it seems See the test: martinmacko47#7 I have put it into a separate branch until we decide it is working, so we won't intermingle two problems here. Edit: Btw, it seems to work for both drives There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've merged martinmacko47#7 both to this PR and to #4444. I've also backported all additional commits from #4444 except the commit using dequeque methods from drivers. So we have both versions ready in case it will take too long to get the dequeque methods upstream to drivers. |
||
return true; | ||
} | ||
return false; | ||
} | ||
} | ||
|
||
Object.assign(Client_MySQL2.prototype, { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to attach some identification to the driver query objects, in order to find out which query to remove from the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could keep a reference to driver's query object:
And then use this reference to identify the driver's query by object identity. E.g.: