-
Notifications
You must be signed in to change notification settings - Fork 299
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
Fix Async Cancel #956
Conversation
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
Outdated
Show resolved
Hide resolved
Looks good to me. Can you bring the changes to netfx too? |
Netfx added. |
The failing TVP Test is an actual error and needs further investigation. |
Great, just one tiny test to debug... |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandCancelTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Show resolved
Hide resolved
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? |
The TVP failure occurs on StreamInputParam.ImmediateCancelBin(). |
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. |
Hi @Wraith2 We'll help you with that, I'm reopening PR so we can help investigate test issues, don't worry! |
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.
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 😉).
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs
Outdated
Show resolved
Hide resolved
…t/TdsParserStateObject.cs Co-authored-by: DavoudEshtehari <61173489+DavoudEshtehari@users.noreply.github.com>
e4f0827
to
afc3429
Compare
+ Test Added for failed usecase
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.