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

`.timeout(ms, {cancel: true})` to cancel query after timeout (only implemented for MySQL) #1454

Merged
merged 6 commits into from May 28, 2016

Conversation

Projects
None yet
3 participants
@bywo
Contributor

bywo commented May 26, 2016

Taking a stab at addressing #707

Adds an optional options argument to .timeout().

For MySQL, a KILL threadId query will get sent. For all other dialects an error will be thrown immediately from the .timeout() call.

While writing tests, I discovered some potentially worrying behavior (comment also added in the test). After a timeout, knex releases the connection back to the pool, but the connection may not be usable because the original slow query is still being performed. A subsequent query will acquire the connection (still in-use) and hang until the first query finishes.

if(isNumber(ms) && ms > 0) {
this._timeout = ms;
if (cancel) {

This comment has been minimized.

@wubzz

wubzz May 26, 2016

Collaborator

Unnecessary nested if-statements here. if (cancel && this.client.canCancelQuery)

if (this.client.canCancelQuery) {
this._cancelOnTimeout = true;
} else {
throw new Error("Query cancelling not supported for this dialect");

This comment has been minimized.

@wubzz

wubzz May 26, 2016

Collaborator

Is this really needed here given the default client function? I would rather have the second argument ignored for other dialects and make a note of dialect support in documentation than duplicating code.

This comment has been minimized.

@bywo

bywo May 26, 2016

Contributor

The default client function won't throw until the query has been sent and already timed out. I felt it'd give a better dev experience if we threw earlier.

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 27, 2016

Collaborator

Would it be possible to make the client-type-agnostic part of knex perform this check by having it provide a wrapper timeout() function that then calls this function only if the check passes? That would kill the duplication and still give us the immediate error feedback.

Haven't checked the relevant knex code though, so might me talking out of my ass here. 😄

if(isNumber(ms) && ms > 0) {
this._timeout = ms;
if (cancel) {

This comment has been minimized.

@wubzz

wubzz May 26, 2016

Collaborator

Unnecessary nested if-statements here. if (cancel && this.client.canCancelQuery)

src/raw.js Outdated
if (this.client.canCancelQuery) {
this._cancelOnTimeout = true;
} else {
throw new Error("Query cancelling not supported for this dialect");

This comment has been minimized.

@wubzz

wubzz May 26, 2016

Collaborator

Is this really needed here given the default client function? I would rather have the second argument ignored for other dialects and make a note of dialect support in documentation than duplicating code.

var query = testQueries[dialect]();
var addTimeout = () => query.timeout(1, {cancel: true})

This comment has been minimized.

@wubzz

wubzz May 26, 2016

Collaborator

Unfortunately we can't es6 in the test suite, it's being run for 0.10 and 0.12 as well.

This comment has been minimized.

@bywo

bywo May 26, 2016

Contributor

👍 will fix

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 26, 2016

I understand the issue here and don't mind the implementation. Would be great to support other dialects as well later on.

What I'm curious about is what would happen if the cancelQuery function is unable to acquireConnection due to the pool being full (or no resources available).

@bywo

This comment has been minimized.

Contributor

bywo commented May 26, 2016

@wubzz thanks for the quick review!

Good point about cancelQuery not being to acquire a connection. I could put a timeout on the cancel part as well and throw a different error, e.g. Unable to cancel query that hit timeout. What do you think?

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 26, 2016

@byronmwong Yeah some form of fallback might be needed. Perhaps the simplest solution would be to have the promise "resolve" itself after timing out. That way the original expected timeout error would still be thrown.

@bywo

This comment has been minimized.

Contributor

bywo commented May 26, 2016

@wubzz yep that could work, but then we're silently failing on the cancel part.

If a dev calls .timeout(ms, {cancel: true}) and receives a TimeoutError, they'd expect the original query to be cancelled.

bywo added some commits May 26, 2016

address wubzz's comments, improvements
* add timeout to acquiring connection inside `cancelQuery` + test
* use `KILL QUERY :id` instead of `KILL :id` so the connection is preserved
var dialect = knex.client.config.dialect;
if(dialect === 'sqlite3') { return done(); } //TODO -- No built-in support for sleeps
if(dialect === 'sqlite3') { return; } //TODO -- No built-in support for sleeps

This comment has been minimized.

@jurko-gospodnetic

jurko-gospodnetic May 27, 2016

Collaborator

you can make sqlite sleep implicitly by using two separate transactions/connections, one locks a database table, and another attempts to access that table, which will cause it to wait for the lock to be released (5 seconds by default, if I recall correctly)

This comment has been minimized.

@bywo

bywo May 27, 2016

Contributor

ohh that's a good idea! @jurko-gospodnetic @wubzz is this a blocker on merging?

@bywo

This comment has been minimized.

Contributor

bywo commented May 27, 2016

@wubzz: added a timeout on acquiring connection for cancelling queries and got tests to pass. ready for another look!

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 27, 2016

Looks good to me. Given that the original timeout test was written by me, I don't think the responsibility of adding sqlite support in the test falls on you, so I don't think it's a blocker for this PR.

A quick search shows that postgres, maria and oracle should have support for this action as well. Is this something to look into in this PR, or would you rather a separate issue?

@bywo

This comment has been minimized.

Contributor

bywo commented May 27, 2016

Turns out the implementation for maria is the same as mysql, so I've
handled that. I'd prefer a separate issue for postgres and oracle
support. Thanks!

On Fri, May 27, 2016 at 3:08 PM wubzz notifications@github.com wrote:

Looks good to me. Given that the original timeout test was written by me,
I don't think the responsibility of adding sqlite support in the test falls
on you, so I don't think it's a blocker for this PR.

A quick search shows that postgres, maria and oracle should have support
for this action as well. Is this something to look into in this PR, or
would you rather a separate issue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1454 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABXjcUTQiy6HcZFmIS_MaHz62_gij0R2ks5qF2txgaJpZM4In3KA
.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented May 27, 2016

@byronmwong That's alright with me, I'll make a new issue for postgres/oracle support.

One last thing I almost forgot before I merge this, could you update the docs (index.html) of timeout to reflect this change? Make sure to specify it only applies to mysql/maria for now.

@bywo

This comment has been minimized.

Contributor

bywo commented May 27, 2016

done! no worries, I should've thought to update the docs myself.

@wubzz wubzz merged commit 27d8282 into tgriesser:master May 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment