Fixed unresolved promise in cancelQuery(..) ... #3666
Conversation
... 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) |
There was a problem hiding this comment.
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.
|
@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) |
|
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. |
|
@elhigu Only other dialect that seem to have |
|
Thank you so much! |
|
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. |
... 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.