Fix cancelling timeouted queued queries in transactions for MySQL

[martinmacko47]

Attempt to fix #4415 by removing the timeouted queries from the driver queue.

[martinmacko47]

We need to attach some identification to the driver query objects, in order to find out which query to remove from the queue.

[martinmacko47]

Alternatively we could keep a reference to driver's query object:

obj.__query = connection.query(...)

And then use this reference to identify the driver's query by object identity. E.g.:

query === queryToKill.__query

[martinmacko47]

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.

[maximelkin]

This is quite an invasion into the driver internals

Dont really like this idea. Can we implement it in drivers first?

[maximelkin]

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

[martinmacko47]

@maximelkin

Can we implement it in drivers first?

I can definitely try to make a PR to drivers. But first we should be sure this solution is acceptable for us.

Wait, implement this probably bad idea, can we try to make things same for pg and mysql instead?

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 ? nor pg_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 ? and pg_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.

[maximelkin]

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

[martinmacko47]

@maximelkin @kibertoad I filed the feature request to both drivers:

• Remove queued query mysqljs/mysql#2478
• Remove queued query sidorares/node-mysql2#1316

Please provide every possible endorsment for this feature request in the drives on behalf of Knex.

[martinmacko47]

@maximelkin @kibertoad I have created draft implementations of Query.dequeue() for both drivers mysql and mysql2:

• Remove queued query mysqljs/mysql#2480
• Remove queued query martinmacko47/node-mysql2#1 (I will create an upstream PR for this one after somebody from mysql2 team responds to the issue)

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 until Query.dequeue() gets merged into the drivers: #4444

[martinmacko47]

@maximelkin

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.

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 after pause() won't start any new queries in the connection until we resume() 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 we resume() 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 as mysql2. Even though they have different implementation for pause()/resume().

[martinmacko47]

I have put it into a separate branch until we decide it is working, so we won't intermingle two problems here.

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.