Cancellable Query with AbortSignal option#3212
Conversation
…ted in the Connection class. Kept the client.ssl prop for now, as that is technically on the public API, but undocumented.
I’m pretty sure there are still unavoidable race conditions that make this not the right API. |
That was my original impression as well. One possible issue I see with this is destroying the connection, if the cancellation connection closes with an error. This is done, so we don't release the connection with a possible cancellation pending. However, if the cancellation actually failed, we just left possibly long running query alive on the server. Explicitly sending an RST might be necessary in this case, assuming that is handled similarly as cancel requests by the server. Even if all this works as I think it does, I'm not so sure if this API is a good idea. The intent of the code:
So depending on timing the query might be canceled, prevented from being executed or complete successfully. But the connection should be left In a clean state, or be destroyed. |
A suggestion for a new cancellation API (based on the original request in #2774)
This query-specific cancellation API only makes sense because the library waits for each query to complete before it submits the next. With query pipelining it would be a lot messier - or simpler, because the API could offer less guarantees.
While the tests are turned off if
AbortControlleris not available, the library code itself should still be compatible with older node versions.Example
First, in a separate commit (913967f) is a small change that moves the SSL initialization logic to the
Connection.This means the
Connectionno longer needs a separate an'sslconnect'event.Then, the
Connectiongot a newcon.cancelWithClone()method that tries to connect to the exact same server, and send a cancel request.This required a bit of extra state in the
Connection, to keep track of thehost,port,configand thebackendKeyData.I'm not thrilled with duplicating state from the
Client(or with the name of the method for that matter).What I like about this approach is that all the logic around the cancellation can be implemented in the
Submittableand doesn't depend on any special handling by theClient.Finally the
Querygot asignaloption that can be used to cancel it either before, or after it has been submitted to the server.If the query has been canceled before submit, it will return the abort reason in submit.
If an actual cancel request has been sent, the query callback and the
'error'and'end'events are delayed until the cancel request has finished.This is done to avoid accidentally canceling a different query on the same connection.
If the cancel request isn't completed cleanly, the original connection is destroyed.
This might be triggered if you get an ECONNRESET after sending the cancel request? Honestly I'm not sure.
This should probably treat the errors after the cancel request has been sent differently from errors during connecting.