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

CancellationToken support? #343

Closed
jakejscott opened this issue Nov 17, 2019 · 22 comments
Closed

CancellationToken support? #343

jakejscott opened this issue Nov 17, 2019 · 22 comments
Assignees
Labels
deployed Feature or bug is deployed at the current release fixed The bug, issue, incident has been fixed. question Further information is requested todo Things to be done in the future

Comments

@jakejscott
Copy link

Hey ya, do you have any plans to support CancellationToken's?

@mikependon mikependon self-assigned this Nov 18, 2019
@mikependon mikependon added the question Further information is requested label Nov 18, 2019
@mikependon
Copy link
Owner

Hi @jakejscott , actually no. But, I really would like to if you can share me the detailed explanation and which scenario. Can you expand this thread to let me understand it further?

@SergerGood
Copy link
Collaborator

@mikependon why not? This is necessary to cancel long-running operations (like batch insert). There must be a way to stop an operation that is no longer needed. Or you will have to wait for its end.

@mikependon
Copy link
Owner

TBH, I know this is quite needed in some cases, but such request has not been triggered after this. The user closes the request ticket without providing enough information. Happy to reopen this if you can provide same samples, scenarios and/or recommendations.

Also, I think I need to spend time researching what happened to the DB Provider side if the operation has been executed already, but a cancel has been requested.

@mikependon mikependon reopened this Sep 25, 2020
@mikependon
Copy link
Owner

This could be related to this dotnet/runtime story.

@mikependon mikependon pinned this issue Sep 26, 2020
@mikependon
Copy link
Owner

@jakejscott @SergerGood - I can easily add the CancellationToken on the Async methods, however, how would you like to handle the sync version?

Remember, you do not have access to the DbCommand object in all kinds of execution within RepoDB, therefore, you cannot call the DbCommand.Cancel() method. Of course, unless we place it to the ClassHandler (#530) object and/or provide a callback function calls.

What's your preference?

@SergerGood
Copy link
Collaborator

It will be like this for example 5580088
And the use is like this

        public async Task<List<Person>> Query(CancellationToken cancellationToken)
        {
            using IDbConnection connection = await new SqlConnection(ConnectionString)
                .EnsureOpenAsync(cancellationToken);

            return await connection.QueryAsync<Person>(x => x.Id == CurrentId, cancellationToken: cancellationToken);
        }

@mikependon
Copy link
Owner

@SergerGood - I am actually doing this one already, but it is only for Async. How about the sync version? How would you do that as we do not have reference to the DbCommand.Cancel().

@SergerGood
Copy link
Collaborator

I have not seen use of undo for synchronous operations when working with DbCommand...

mikependon added a commit that referenced this issue Sep 28, 2020
@SergerGood
Copy link
Collaborator

May i ask why not used default?

@mikependon
Copy link
Owner

I think to force it to be nullable instead for the reason of overloaded methods. There might be case that the user will never ever pass a value to it, and placing a default would force the execution to an overloaded method of the underlying DbCommand.

I used to do it this way.

using (var reader = cancellationToken.HasValue ? await command.ExecuteReaderAsync(cancellationToken.Value) : await command.ExecuteReaderAsync())
{
     ...
}

And by having a default, would force it to the await command.ExecuteReaderAsync(cancellationToken), in which the value of the cancellationToken is the CancellationToken.None. I guess it should not be like that?

@SergerGood
Copy link
Collaborator

CancellationToken.None is the default way to pass empty CancellationToken.
If CancellationToken.None used then that is, its CanBeCanceled property is false. https://github.com/microsoft/referencesource/blob/f461f1986ca4027720656a0c77bede9963e20b7e/System.Data/System/Data/Common/DBCommand.cs#L250 The user does not need to pass a value by default, and the behavior is handled correctly.

@mikependon
Copy link
Owner

Yeah, in such a case, you can always pass it on the nullabe argument right? The behavior is to make sure not affecting as well all the previous implementations.

@SergerGood
Copy link
Collaborator

If used as a parameter with a default value, it has no effect for all the previous implementations.

..., CancellationToken cancellationToken = default)

@mikependon
Copy link
Owner

@SergerGood - you are correct on pushing this, they are passing the default value here. Great sleuthing!

@mikependon
Copy link
Owner

Relating the recent found bugs at #601.

@mikependon
Copy link
Owner

@SergerGood - I have to remove the explicit calls you made for cancellationToken.ThrowIfCancellationRequested();. Let us just pass it and have RepoDB bubble ups the exception thrown by ADO.NET itself. Hope it is okay.

@SergerGood
Copy link
Collaborator

@mikependon - I think that it is ok)

@mikependon
Copy link
Owner

@SergerGood - I am done with the changes, but we have some failing Unit and Integration Tests. Will work on this soon. In the meanwhile, would you be able to do a double check while I am fixing the failing tests? Thanks

mikependon added a commit that referenced this issue Oct 3, 2020
#343 Fixed the failing Integration Tests for SqLite and MySQL.
@mikependon mikependon added deployed Feature or bug is deployed at the current release fixed The bug, issue, incident has been fixed. labels Oct 3, 2020
@mikependon
Copy link
Owner

This is now available at RepoDB v1.12.4. Please see the actual release here.

mikependon added a commit that referenced this issue Oct 4, 2020
The impact to this is very minimal, or almost 0. I just finalized this feature to ensure we do not have technical debt in relation to the CancellationToken and cancellable Async calls.
@mikependon mikependon unpinned this issue Oct 6, 2020
@mikependon
Copy link
Owner

Setting this one to close now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed Feature or bug is deployed at the current release fixed The bug, issue, incident has been fixed. question Further information is requested todo Things to be done in the future
Projects
None yet
Development

No branches or pull requests

3 participants