You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thinking about this again after refactoring cancellation...
PerformPostgresCancellation should probably always return immediately. Since PG cancellation isn't guaranteed to work, is completely out-of-band (on a different connection), and isn't support to throw any exceptions, there is no reason to wait for the send to complete. We can simply perform the operation asynchronously and not await it.
This is important because PerformPostgresCancellation is called when the user's cancellation token is triggered, which means that when we dispose CancellationTokenRegistration, we have to wait (synchronously or asynchronously) until the callback completes. It's difficult to do this - CancellationTokenRegistration isn't IAsyncDisposable in netstandard2.0, and even when it is we have an async flag to check. We could do all this, but there really isn't any reason.
One note on the above: not waiting for the cancellation attempt to complete could mean that we miss a write exception, which does affect our flow a bit. However, that doesn't seem important - in the worse case, we'll think a cancellation attempt succeeded when it hasn't, and wait two seconds before doing hard cancellation; that seems reasonable.
Not really - we wait, until pg closes the cancellation connection.
But how does that prevent race conditions? The cancellation token (or NpgsqlCommand.Cancel) can be called at any time, from any thread, and nothing guarantees when the cancel packet arrives at PG or when it applies it... Also, it's the user's responsibility to properly wait for the 1st command to finish (either normally or with a cancellation exception), before a second command is sent, no?
(I know that there's a comment about this in DoCancelRequest - but I'm not sure it's good behavior)
OK, yeah - I forgot that we wait for PG to actually close the connection, so it's reasonable to expect that the cancellation has already been applied. I will scope this out of my refactor for now.
Activity
roji commentedon Nov 11, 2020
Thinking about this again after refactoring cancellation...
PerformPostgresCancellation should probably always return immediately. Since PG cancellation isn't guaranteed to work, is completely out-of-band (on a different connection), and isn't support to throw any exceptions, there is no reason to wait for the send to complete. We can simply perform the operation asynchronously and not await it.
This is important because PerformPostgresCancellation is called when the user's cancellation token is triggered, which means that when we dispose CancellationTokenRegistration, we have to wait (synchronously or asynchronously) until the callback completes. It's difficult to do this - CancellationTokenRegistration isn't IAsyncDisposable in netstandard2.0, and even when it is we have an async flag to check. We could do all this, but there really isn't any reason.
roji commentedon Nov 11, 2020
One note on the above: not waiting for the cancellation attempt to complete could mean that we miss a write exception, which does affect our flow a bit. However, that doesn't seem important - in the worse case, we'll think a cancellation attempt succeeded when it hasn't, and wait two seconds before doing hard cancellation; that seems reasonable.
vonzshik commentedon Nov 11, 2020
Wouldn't it lead to possible concurency issue, when the request is send at the end of a query, and it cancels something else?
roji commentedon Nov 11, 2020
This can always happen regardless of this, no?
roji commentedon Nov 11, 2020
https://www.postgresql.org/message-id/CADT4RqD2h199isLd7wDVm_ogBnWn0FgKtYBqLEdn-QZh3bGP6A%40mail.gmail.com
vonzshik commentedon Nov 11, 2020
Yeah, I saw this.
Not really - we wait, until pg closes the cancellation connection.
vonzshik commentedon Nov 11, 2020
But it can happen for the
NpgsqlCommand.Cancel.roji commentedon Nov 11, 2020
But how does that prevent race conditions? The cancellation token (or NpgsqlCommand.Cancel) can be called at any time, from any thread,
and nothing guarantees when the cancel packet arrives at PG or when it applies it... Also, it's the user's responsibility to properly wait for the 1st command to finish (either normally or with a cancellation exception), before a second command is sent, no?(I know that there's a comment about this in DoCancelRequest - but I'm not sure it's good behavior)
roji commentedon Nov 11, 2020
OK, yeah - I forgot that we wait for PG to actually close the connection, so it's reasonable to expect that the cancellation has already been applied. I will scope this out of my refactor for now.
roji commentedon Nov 13, 2020
@vonzshik I propose we leave this for 6.0 - we will have cancellation-related leftovers in any way, does that sound OK?
vonzshik commentedon Nov 13, 2020
ok