Skip to content

query cancel #1392

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

Closed
wants to merge 8 commits into from
Closed

query cancel #1392

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5810fbb
query cancel
Jul 31, 2017
b1f5aa7
query cancel fix
Aug 23, 2017
d50159c
cancel moved to Query api
Sep 5, 2017
63b6ad9
lint fix
Sep 5, 2017
b76ca42
native query cancel
Sep 6, 2017
039b5ba
callback as arrow function
Oct 9, 2017
cb3d4ae
query_canceled error code defined as constant
Oct 9, 2017
dee5a59
check as late as possible if we wont cancel another query (see https:…
Oct 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,22 +276,20 @@ Client.prototype.getStartupConf = function () {
return data
}

Client.prototype.cancel = function (client, query) {
if (client.activeQuery === query) {
var con = this.connection

if (this.host && this.host.indexOf('/') === 0) {
con.connect(this.host + '/.s.PGSQL.' + this.port)
} else {
con.connect(this.port, this.host)
}

// once connection is established send cancel message
con.on('connect', function () {
con.cancel(client.processID, client.secretKey)
Client.prototype.cancel = function (query) {
if (this.activeQuery === query) {
// we need a new connection to the server (without StartupMessage)
// https://www.postgresql.org/docs/9.3/static/protocol-flow.html#AEN99752
// we will use the connection of a new client...
const cancelingClient = new Client(this.connectionParameters)
const connection = cancelingClient.connection
connection.connect(this.port, this.host)
const self = this
connection.on('connect', function () {
connection.cancel(self.processID, self.secretKey)
})
} else if (client.queryQueue.indexOf(query) !== -1) {
client.queryQueue.splice(client.queryQueue.indexOf(query), 1)
} else if (this.queryQueue.indexOf(query) !== -1) {
this.queryQueue.splice(this.queryQueue.indexOf(query), 1)
}
}

Expand Down Expand Up @@ -379,6 +377,8 @@ Client.prototype.query = function (config, values, callback) {
rejectOut = reject
})
query.callback = (err, res) => err ? rejectOut(err) : resolveOut(res)
} else {
result = query
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn’t be added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need a reference to the query to be able to cancel it later. Is this breaking something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, it was removed intentionally and doesn’t work with promises. I’m not sure what the right fix is, though… requiring the Query object to be passed explicitly like in the tests? Or a separate method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was that if you want to cancel a request, you don't use the promise api. Most of users don't cancel queries, the few people who need to cancel will have to use the callback api. That seemed to me to be the best tradeoff.

In my application I have written a wrapper around the callback api to use promises. Cancelable queries resolves with something like:

{
  data: Promise { <pending> },
  cancel: [Function: bound cancel]
}

But this is more complex for users; the cancelable queries api become different from the "normal" queries; and maybe it's not the pg responsibility to do this kind of things (too high level?). And the wrapper itself needs to keep references to query...

Do you remember why returning the query was removed?

}
}

Expand Down
26 changes: 26 additions & 0 deletions test/integration/client/cancel-query-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict'
var helper = require('./test-helper')
var Query = helper.pg.Query

// before running this test make sure you run the script create-test-tables
test('simple query interface', function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test doesn’t accept a done callback or return a promise, so its assertions won’t fail at the right time. The assert(true) can’t fail at all and doesn’t confirm that row is called. Both queries are queued, so the important cancellation path isn’t tested at all. (Actually, the first one is queued too, because the test doesn’t wait for the client to connect.)

Queries spliced out of the queue should probably reject/emit an error like they would if cancelled while running. Then this test can use the promise API and:

  • wait for the connection to complete
  • start a query, asserting that it resolves
  • start two more queries, cancelling them both and asserting that both reject

var client = helper.client()
client.on('drain', client.end.bind(client))

var query3 = client.query(new Query('select pg_sleep(3) as sleep3'))
query3.on('row', function (row, result) {
assert(true)
})

var query5 = client.query(new Query('select pg_sleep(5) as sleep5'))
client.cancel(query5);
query5.on('row', function (row, result) {
assert(false)
})

var query7 = client.query(new Query('select pg_sleep(7) as sleep7'))
client.cancel(query7);
query7.on('row', function (row, result) {
assert(false)
})
})