-
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
Open
martinmacko47
wants to merge
5
commits into
knex:master
Choose a base branch
from
martinmacko47:promise-all-timeout-error+fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
6fb24fc
Add test for promise all timeout error in transactions
martinmacko47 d860f11
Fix promise all timeout error in transactions
martinmacko47 53fabf1
Use identity comparision to find queued query
martinmacko47 b8dbd32
Add test to not cancel earlier queries in a transaction before the ti…
martinmacko47 8c89de4
Use connection pause/resume to prevent kill race condition (#7)
martinmacko47 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 comment
The 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 comment
The 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?
Also some related thread was here #4324
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.
@maximelkin
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:
KILL QUERY ?
stops the current query, and keeps both the transaction and the connection active. The user may continue with and eventually commit the transaction.KILL CONNECTION ?
stops the current query, rolls back the transaction and closes the connection.In Postgres there are also two commands:
pg_cancel_backend(?)
stops the current query, marks the transaction aborted, bug keeps the connection active. The user can roll back to a savepoint and continue with the transaction, or roll back the transaction completly.pg_terminate_backend(?)
stops the current query, rolls back the transaction and closes the connection.We probably don't want to use
KILL CONNECTION ?
norpg_terminate_backend(?)
because that would be a breaking change for timeouts outside transactions. Timeouts outside transactions worked for years already, so there will be a lot of code expecting connections won't be closed after a query times out.So we have to use
KILL QUERY ?
andpg_cancel_backend(?)
which have different semantics. We can't make them do the same. We just need to use them somehow to make the timeouts working reliably also in transactions.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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
@maximelkin @kibertoad I have created draft implementations of
Query.dequeue()
for both driversmysql
andmysql2
:Also I have adapted this PR to use
Query.dequeue()
instead of driver internals. I created a separate PR for it, so we can have both versions arround untilQuery.dequeue()
gets merged into the drivers: #4444There 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.
@maximelkin
I managed to simulate this race condition in a test. And it seems
connectionToKill.pause()
/resume()
indeed fixes it. Even thought it was supposedly meant for streams, it actually pauses processing packets from server for non-stream queries as well. As a consequence the driver afterpause()
won't start any new queries in the connection until weresume()
it. Therefore even if the timeouted query happens to complete before we manage to kill it, no new query will start in the transaction, until weresume()
the connection.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
mysql
as well asmysql2
. Even though they have different implementation forpause()
/resume()
.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.
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.