Skip to content
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

Fixed unresolved promise in cancelQuery(..) ... #3666

Conversation

@briandamaged
Copy link
Collaborator

briandamaged commented Feb 15, 2020

... more specifically: 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.

... more specifically: 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.
// 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)

This comment has been minimized.

Copy link
@briandamaged

briandamaged Feb 15, 2020

Author Collaborator

Short story: I remove the 100ms timeout here.

I think the logic was intended to "cancel the cancellation" if it took too long. In reality, it was just returning control to the caller after 100ms; yet the actual cancellation operation was still running.

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 15, 2020

@maximelkin + @kibertoad + @bywo : Proposing this as the fix for the "timeout" issue that was being observed. Although the PR at #3665 makes the unhandled promise issue go away, it is ultimately just obscuring the underlying problem. (the underlying problem being: there is an exception that cannot be caught/handled by the caller)

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 15, 2020

This is very old code from 2016, wonder if it still makes sense with how Tarn.js works.
@elhigu @koskimas Any thoughts? For me new behaviour makes more sense, but this is obviously a pretty sensitive part of the code.

@elhigu

This comment has been minimized.

Copy link
Member

elhigu commented Feb 16, 2020

Sounds like the reason why timeout was there was to prevent case where connection could not have been acquired for cancelation.

This implementation looks fine to me. AFAIK Tarn's acquire connection timeout should automatically handle this case as it is used in this fix.

However it should be made sure that other dialect cancelation methods are fixed as well.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 16, 2020

@elhigu Only other dialect that seem to have cancelQuery implementation is Postgre, and it doesn't have this kind of logic already, which would explain why we were getting this test fail only on mysql.

@kibertoad kibertoad merged commit 31e5418 into knex:master Feb 16, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Feb 16, 2020

Thank you so much!

@briandamaged

This comment has been minimized.

Copy link
Collaborator Author

briandamaged commented Feb 16, 2020

np! I'm just glad we were able to find the cause for the intermittent failures!

Thx to @maximelkin as well for helping to brainstorm about possible race conditions / etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.