-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Register cmd.Cancel() to token #1590
base: main
Are you sure you want to change the base?
Conversation
Allow users to extend cancellation past the point of beginning execution during asynchronous operations. By registering cmd.Cancel() to the cancellationToken, this allows the command execution to be halted after it has started returning results. This is particularly beneficial for long running queries.
IDapperConnectionAsync connection = new YourConnectionHere().GetDapperConnectionAsync(); |
A couple of thoughts on this. Firstly, adding parameters: not that simple - that's a binary breaking change. If we needed to add a Boolean flag, I wonder if we could perhaps instead use the flags enum that is exposed elsewhere. However, I'm ... suspicious about this change. The DbCommand async methods already take a cancellation token, IIRC, and we should (although mistakes do happen) already be passing in that token. On an async API, I would expect the ADO.NET provider to handle async cancellation internally based upon the token passed in. Does this not already happen? |
With the current settings, not including the proposed changes, lets say theres a long running query that takes 17 seconds to get initial results but wont finish collecting results until 1:00. Using a cancellationToken it is possible to cancel the query within that first 17 seconds. It is also possible to set the command timeout to < 17 and it will "time out" canceling the query with similar behavior. However, if the command timeout settings is 30 or a cancellation is requested via cancellationToken after the initial results are returned the execution continues until completion. Using the Flags enum should be feasible in lieu of adding a new parameter. Both IDbCommand and SqlCommand have the Cancel() method which will halt execution regardless of how far it has gone. Passing in a cancellation request via token, does not have the same exact effect, but will still cancel if you request it early enough. |
I've been working with @ijarvis11 on this, and one thing to note is we're both new to cancellation tokens / async operations, but I think where Dapper might be a bit off here is that there is no overload for Query that will accept a SqlCommand, where one would register the the SqlCommand.Cancel() method on CancellationToken.Token. https://stackoverflow.com/questions/24738417/canceling-sql-server-query-with-cancellationtoken (the first answer here, seems to be what we cannot do with a CommandDefinition as the only overload.) A CommandDefinition doesn't seem to have a cancel method that can be utilized as the Token.Register registration like an IDbCommand/SqlCommand would. Assuming there isn't a solid reason for keeping it exactly as it is on release, I believe the flag option might work. When passing in a Command Definition, it ends up in something like this: private static async Task<IEnumerable> QueryAsync(this IDbConnection cnn, Type effectiveType, CommandDefinition command) This gets us almost all the way there, the Cancellation token does get passed down through to the connection, but as we understand it, it's missing one line after the instantiation of the var cancel before you try to run on the cmd. cancel.Token.Register(() => cmd.Cancel()); If there is a true difference in the way the cancellation token works in this setup, I think this is what gets us the rest of the way there. We may be making incorrect assumptions about how cancellation works, so we're open to being wrong on this. |
I need to do a little research in the expected behaviour here - already
fired off a ping on that topic. However, two other observations:
- the registration needs disposing (note it should be kept as the struct,
not boxed to IDisposable)
- the registration should use the state object overload to avoid a capture
allocation; if possible, the new "static lambda" functionality would be
used (this proves that you haven't captured anything)
…On Fri, 4 Dec 2020, 20:57 Matthew Stewart, ***@***.***> wrote:
I've been working with @ijarvis11 <https://github.com/ijarvis11> on this,
and one thing to note is we're both new to cancellation tokens / async
operations, but I think where Dapper might be a bit off here is that there
is no overload for Query that will accept a SqlCommand, where one would
register the the SqlCommand.Cancel() method on CancellationToken.Token.
https://stackoverflow.com/questions/24738417/canceling-sql-server-query-with-cancellationtoken
(the first answer here, seems to be what we cannot do with a
CommandDefinition as the only overload.)
A CommandDefinition doesn't seem to have a cancel method that can be
utilized as the Token.Register registration like an IDbCommand/SqlCommand
would.
Assuming there isn't a solid reason for keeping it exactly as it is on
release, I believe the flag option might work. When passing in a Command
Definition, it ends up in something like this:
private static async Task<IEnumerable> QueryAsync(this IDbConnection cnn,
Type effectiveType, CommandDefinition command)
{
object param = command.Parameters;
var identity = new Identity(command.CommandText, command.CommandType, cnn,
effectiveType, param?.GetType());
var info = GetCacheInfo(identity, param, command.AddToCache);
bool wasClosed = cnn.State == ConnectionState.Closed;
var cancel = command.CancellationToken;
using var cmd = command.TrySetupAsyncCommand(cnn, info.ParamReader);
DbDataReader reader = null;
try
{...
}
This gets us almost all the way there, the Cancellation token does get
passed down through to the connection, but as we understand it, it's
missing one line after the instantiation of the var cancel before you try
to run on the cmd.
cancel.Token.Register(() => cmd.Cancel());
If there is a true difference in the way the cancellation token works in
this setup, I think this is what gets us the rest of the way there.
We may be making incorrect assumptions about how cancellation works, so
we're open to being wrong on this.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1590 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEHME57JNHDJBBHG4TGGTSTFELPANCNFSM4UNQUPVQ>
.
|
Pulling out previous addition of new parameter. Will add new flag enum to CommandFlags.cs
Initial response to my question to ADO.NET folks is that a cancellation-token should already work this way, so; this leaves 3 possibilities as I see it:
I can check 1, but: what exact ADO.NET provider are you seeing this with? So we can check whether the problem is "2". |
To add to what @mgravell wrote, #1590 (comment) makes me think that the concern here is with cancellation after the initial ExecuteReaderAsync has completed (but results are still coming back). For this scenario, my expectation would be for any of the subsequent methods on the reader (ReadAsync, NextResultAsync) to cancel the query if passed a cancelled token. To step back, a complex cancellable operation - governed by a single cancellation token - may involve multiple database queries, as well as other cancellable non-database operations. It's important for the application to be able to signal cancellation once on the token, and have that work without any extra work, such as registering callbacks on the token to handle custom logic, etc. For one thing, this PR does something specifically for Dapper that, if actually problematic, affects non-Dapper users as well - so we should consider this with that scope in my mind. FYI I've written some thoughts on this in this blog post. |
The specific provider we are using is SqlDataReader. |
@roji just to clarify: are you saying that if it isn't already cancelling cleanly (assuming I haven't missed a token in one of the calls), then that should be fixed in ADO.NET / SqlClient, rather than Dapper? While that may be true, being pragmatic that is going to take a long time to get done, and won't apply backwards - where-as a fix in a tool like Dapper: applies everywhere right now. |
@mgravell, unless I'm missing something, it does look like CancellationToken does get equipped all the way through to the execution of the command (at least for QueryAsync() which chains down into ExecuteReaderWithFlagsFallbackAsync()) After reading the article written/linked by @roji, it sounds like what might be happening is our Cancel command tied to a button might be sending the cancellation after the command has actually completed but before the results would have been returned. In essence, maybe our query is running too quickly for us to actually cancel it while it's still running? It might explain the behavior seen, but it's unclear to me. |
Which to me sounds like ReadAsync etc isn't handling the cancellation correctly. In a way I can empathize - ReadAsync is called many times, and it doesn't want to be constantly registering/unregistering a cancellation callback. That's actually why I'm wondering if it might be better for Dapper to handle the cancellation and pass down a default (non-cancelling) token instead. |
@Omnideth cancellation has inherent race conditions - if the query completed and the results are being streamed back from the server, then it's entirely possible that there's nothing more left to cancel; in that case I'd expect DbCommand.Cancel to not do anything either.
I think we probably need to find out exactly what's going on with SqlClient and ReadAsync (which, incidentally, isn't really async). Are we sure that registering DbCommand.Cancel on the cancellation token would actually fix the problems above?
Do you mean that from a performance standpoint? A couple thoughts on that:
All the above is just my thoughts (and how Npgsql works/could work), and doesn't indicate how SqlClient actually works. I definitely understand the pragmatic argument of not waiting for SqlClient to change or fix its behavior - but it's a question of to what extent Dapper wants to be a layer to cover for the shortcomings of one specific ADO.NET driver. On the flip side of the argument, there may not be a big downside to registering Cancel once at the Dapper level. /cc @vonzshik who's probably interested in this conversation. |
Keep in mind: if you register on an already cancelled token, the registration will complete synchronously. That means, no query will ever be cancelled (as it hasn't run yet). |
So: if (token.IsCancellable)
{
disposeLater = token.Register(.... Cancel ...);
token.ThrowIfCancellationRequested();
} Or similar; doable, but a very valid point |
But what if it's cancelled just after |
Fine, we can check again after it gets sent and before the read; we can check the heck out of that token. That's the great thing about library code: we can add the stupid levels of checks that nobody in their happy mind would add in application code. |
|
@vonzshik on one level I agree with you, but the premise of this thread (one that I haven't yet plumbed the depths of, so I'm not claiming it as absolute) is that SqlClient isn't doing that correctly, and that others may not either. So while I agree from a purism standpoint, I'm fundamentally a pragmatist, and the pragmatic reality is that if the providers aren't going to do it properly, that leaves two options:
1 seems preferable to 2, in the absence of the ideal solution (providers doing it) But first, I need to check that it isn't Dapper's bug in the first place, i.e. us missing a parameter or seven |
I know we've gone back and forth on this quite a bit - but I'm not quite sure what you mean here. My only proposal here, was that if we consider 50ns too much overhead (registering/unregistering the cancellation token when it's cancellable), we could avoid doing that when ReadAsync is called, but data is already buffered. This would indeed mean that if the cancellation token is triggered and then passed to ReadAsync, that would no longer cancel the query at the server as we do today - instead the query would be cancelled only when we run out of buffered data and have to do some I/O. I'm not sure that would be a problem, but I'm also not sure 50ns are a problem :) Apart from that yeah, it seems we should confirm there's an actual problem with SqlClient before considering something at the Dapper level... Let me know if I can help. |
@roji I was talking about something like this: var registration = token.Register(...);
command.ExecuteReaderAsync(); This will lead to registration executing synchronously, so there will be no query cancellation. |
@vonzshik I still don't follow... if a cancelled token gets passed to ExecuteReaderAsync, no query should ever be started, so nothing left to cancel, right? On the other hand, ReadAsync is different since we know a query is already in progress. In any case, if this is about my proposal to stop registering/unregistering in ReadAsync when the next row is already buffered, then I guess the details can be deferred - this was more an idea at this point than something we want to do right away. |
@roji so, I did take a look at the proposal (and the codebase). Yeah, you're correct - the cancellation token is passed to the command. So everything should be more or less fine. |
Thanks @vonzshik. I've taken a quick look at SqlClient's ReadAsync implementation, and it does seem to properly respect cancellation tokens. It even seems to have a fast path for when the next row is buffered, and which bypasses registration; but if data isn't available, cancellation is properly registered. The one issue I'm seeing is exactly what @vonzshik and I discussed re Npgsql, and which is detailed in my post: if the token is cancelled before being passed to ReadAsync, ReadAsync will return a cancelled task immediately, but the query will continue running (Npgsql now cancels the query for this case). I think this behavior isn't very useful, as it's very prone to race conditions and the exact timing the token is triggered. This could be a reason to set up a callback at the Dapper level, though let me discuss things internally a bit to see what people think. |
Dapper/CommandDefinition.cs
Outdated
@@ -126,6 +131,8 @@ internal IDbCommand SetupCommand(IDbConnection cnn, Action<IDbCommand, object> p | |||
if (CommandType.HasValue) | |||
cmd.CommandType = CommandType.Value; | |||
paramReader?.Invoke(cmd, Parameters); | |||
if (RegisterCancellation) | |||
CancellationToken.Register(() => cmd.Cancel()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be cancellationToken.Register(static state => ((IDbCommand)state).Cancel(), cmd);
to avoid a capture context and delegate allocation, however:
- honestly, I think we should pause on this a sec while we discuss the overall strategy and API intent
- we'd also need to think about how to handle the
CancellationTokenRegistration
(return value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. I will wait to see what fruits the pending discussions bear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if/when this would next be looked at, but I didn't want to leave the code is a bad place. I went ahead and applied the updates you suggested and implemented a way to dispose the CancellationTokenRegistration once its no longer needed.
Dapper/CommandFlags.cs
Outdated
/// <summary> | ||
/// Should the cancellation be allowed after result collection starts? | ||
/// </summary> | ||
RegisterCancellation = 5, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be 8
, not 5
- it is a flags enum; alternatively: Buffered = 1 << 0
, Pipelined = 1 << 1
, NoCache = 1 << 2
, RegisterCancellation = 1 << 3
SqlMapper.Async>ExecuteWrappedReaderImplAsync has the DbCommand which is equipped with the Disposed event that I will use to dispose of the CancellationTokenRegistration.
If user decides to register cancellation, then dispose of registration when the DbCommand is disposed. If users place their DapperReader in a using block, this will dispose of the registration as well once the command is disposed.
Just noting here - overlap with #558, though it does address |
FWIW I opened dotnet/SqlClient#1065 to consider handling this in SqlClient itself, similar to how Npgsql does it: if ReadAsync is called with a cancelled token, cancel the query at the server instead of just immediately returning a cancelled Task. An alternative strategy would be to continue monitoring the cancellation token originally passed to ExecuteReaderAsync - even after ExecuteReaderAsync itself has already completed. This would have the advantage of allowing immediate cancellation at the server at any point, and not only when ReadAsync is called. The bottom line is that ideally Dapper shouldn't need to handle this, as it's a general problem for anyone using SqlClient (though pragmatically-speaking I definitely get why you'd want to do it...). |
Allow users to extend cancellation past the point of beginning execution during asynchronous operations. By registering cmd.Cancel() to the cancellationToken, this allows the command execution to be halted after it has started returning results. This is particularly beneficial for long running queries.