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

Fix Async Cancel #956

Merged
merged 10 commits into from
Sep 17, 2021
Merged

Fix Async Cancel #956

merged 10 commits into from
Sep 17, 2021

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Mar 3, 2021

fixes #44

This removes two contending locks and adds an interlocked state int similar to the way that timeouts were recently changed. This prevents waiting and allows cancel to proceed only if end of execution hasn't started otherwise cancel is ignored.

There was already an async cancel test which was passing so I added a new one based on the repro from the original thread.

@johnnypham
Copy link
Contributor

Looks good to me. Can you bring the changes to netfx too?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jun 26, 2021

Netfx added.

@cheenamalhotra
Copy link
Member

The failing TVP Test is an actual error and needs further investigation.
Shouldn't be treated as random failure as it's failing in all test pipelines.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 5, 2021

Great, just one tiny test to debug...

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 17, 2021

I've put some time into working through TvpMain but I can't work out what the tests are supposed to be doing so I can't isolate what I might have got wrong. Any chance of some help on this?

@DavoudEshtehari
Copy link
Member

The TVP failure occurs on StreamInputParam.ImmediateCancelBin().

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jul 20, 2021

I think I'm going to give up on this. Tests that work locally don't work in the CI, things that work in CI don't work locally and the bizarre threading nature of cancellation makes trying to track down and juggle the exact steps to get all the errors matching up is baroque and frustrating. I'll leave it to the professionals.

@Wraith2 Wraith2 closed this Jul 20, 2021
@cheenamalhotra
Copy link
Member

Hi @Wraith2

We'll help you with that, I'm reopening PR so we can help investigate test issues, don't worry!

Copy link
Member

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Wraith2, After checking the test failures I noticed TdsParserStateObject.Cancel doesn't get into the if-block that will be fixed by applying the recent changes on netfx (be careful about the confusing CompareExchange because the current values of the current and new parameters should be swapped 😉).

…t/TdsParserStateObject.cs

Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview2 milestone Aug 31, 2021
Davoud Eshtehari added 2 commits August 31, 2021 18:10
Fix
The mistaken commits were rolled back and was merged with the current source code.
@cheenamalhotra cheenamalhotra merged commit 576fadb into dotnet:main Sep 17, 2021
@Wraith2 Wraith2 deleted the rework-cancel branch September 17, 2021 23:57
cheenamalhotra added a commit to cheenamalhotra/SqlClient that referenced this pull request Oct 15, 2021
+ Test Added for failed usecase
cheenamalhotra added a commit that referenced this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Canceling SQL Server query with while loop hangs forever
5 participants