Skip to content

Fix Async Cancel - #956

Merged
cheenamalhotra merged 10 commits into
dotnet:mainfrom
Wraith2:rework-cancel
Sep 17, 2021
Merged

Fix Async Cancel#956
cheenamalhotra merged 10 commits into
dotnet:mainfrom
Wraith2:rework-cancel

Conversation

@Wraith2

@Wraith2 Wraith2 commented Mar 3, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@Wraith2

Wraith2 commented Jun 26, 2021

Copy link
Copy Markdown
Contributor Author

Netfx added.

@cheenamalhotra

Copy link
Copy Markdown
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

Wraith2 commented Jul 5, 2021

Copy link
Copy Markdown
Contributor Author

Great, just one tiny test to debug...

@Wraith2

Wraith2 commented Jul 17, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

The TVP failure occurs on StreamInputParam.ImmediateCancelBin().

@Wraith2

Wraith2 commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Member

Hi @Wraith2

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

@DavoudEshtehari DavoudEshtehari left a comment

Copy link
Copy Markdown
Contributor

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
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