Skip to content

Commit

Permalink
Fixed unresolved promise in cancelQuery(..) ... (#3666)
Browse files Browse the repository at this point in the history
cancelQuery(..) was attempting to
"cancel the cancellation" after 100ms.  However, it was not
actually achieving this objective.  In reality, the cancellation
was still running in the background even though the caller had
already moved on.

Later on, the cancellation would ACTUALLY fail due to a resource
allocation issue (ie: no more connections in the Tarn pool).
This would then result in an unhandled Promise rejection.
  • Loading branch information
briandamaged committed Feb 16, 2020
1 parent 77df705 commit 31e5418
Showing 1 changed file with 11 additions and 21 deletions.
32 changes: 11 additions & 21 deletions lib/dialects/mysql/index.js
Expand Up @@ -4,7 +4,6 @@ const inherits = require('inherits');
const { map, defer } = require('lodash');
const { promisify } = require('util');
const Client = require('../../client');
const { timeout } = require('../../util/timeout');
const Bluebird = require('bluebird');

const Transaction = require('./transaction');
Expand Down Expand Up @@ -171,27 +170,18 @@ Object.assign(Client_MySQL.prototype, {

canCancelQuery: true,

cancelQuery(connectionToKill) {
const acquiringConn = this.acquireConnection();

// Error out if we can't acquire connection in time.
// Purposely not putting timeout on `KILL QUERY` execution because erroring
// early there would release the `connectionToKill` back to the pool with
// a `KILL QUERY` command yet to finish.
return timeout(acquiringConn, 100)
.then((conn) =>
this.query(conn, {
method: 'raw',
sql: 'KILL QUERY ?',
bindings: [connectionToKill.threadId],
options: {},
})
)
.finally(() => {
// NOT returning this promise because we want to release the connection
// in a non-blocking fashion
acquiringConn.then((conn) => this.releaseConnection(conn));
async cancelQuery(connectionToKill) {
const conn = await this.acquireConnection();
try {
return await this.query(conn, {
method: 'raw',
sql: 'KILL QUERY ?',
bindings: [connectionToKill.threadId],
options: {},
});
} finally {
await this.releaseConnection(conn);
}
},
});

Expand Down

3 comments on commit 31e5418

@kibertoad
Copy link
Collaborator

Choose a reason for hiding this comment

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

@briandamaged Btw, if you would ever become interested in joining the glorious ranks of knex maintainers, I'd be honoured to invite you in, you really know your stuff :)

@briandamaged
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kibertoad : Thx! I probably take you up on that offer in another week or two. I'd like to tackle a few more smaller merge requests first to make sure I have a better understanding of the "big picture" around Knex.

@briandamaged
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kibertoad : I might take you up on "Becoming a maintainer" if the offer is still open.

Please sign in to comment.