Reimplement rework cancel

[Wraith2]

Reimplementation of #956 to see what fails and why. The extra test added in #1352 passes locally.

/cc @cheenamalhotra in case you're interested or can help pinpoint what's going on here

[codecov]

Codecov Report

Attention: Patch coverage is 71.42857% with 14 lines in your changes missing coverage. Please review.

Project coverage is 63.87%. Comparing base (3607fe2) to head (4f8e8e8).
Report is 268 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs	12.50%	14 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3607fe2) and HEAD (4f8e8e8). Click for more details.

HEAD has 3 uploads less than BASE

Flag	BASE (3607fe2)	HEAD (4f8e8e8)
netfx	2	1
netcore	2	1
addons	2	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2095      +/-   ##
==========================================
- Coverage   70.67%   63.87%   -6.81%     
==========================================
  Files         306      306              
  Lines       61995    61961      -34     
==========================================
- Hits        43816    39578    -4238     
- Misses      18179    22383    +4204     

Flag	Coverage Δ
addons	0.00% <ø> (-92.89%)	⬇️
netcore	67.16% <100.00%> (-6.25%)	⬇️
netfx	62.62% <30.00%> (-6.72%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cheenamalhotra]

I started taking a look too, will find time sometime soon to review this. I was also thinking of an alternative way to pass cancellation token via Async context all the way to TdsParser.. Have you considered that as an option?

Ideally, I would expect Parser to stop executing further as soon as cancellation is requested (switch to Attention mode, same as a timeout) and as lock is released we can have Cancel execute just fine!

[Wraith2]

The CI failed on the test that was added to show that the rework was flawed. I spent some time trying to identify what's going wrong just by reading the test and looking at the code. This might be wrong because i can't reproduce the test failure locally.

I think that the inner connection is closed but still needs to drain the packet data that it has available. It is put into the connection pool and picked up again causing the old packet data to available to the new query which we see as a difference in the query result output parameter.

What I could really do with is a reliable reproduction. I'm going to spin up an azure db over the weekend if I can to see if latency allows me to see the problem locally.

I started taking a look too, will find time sometime soon to review this. I was also thinking of an alternative way to pass cancellation token via Async context all the way to TdsParser.. Have you considered that as an option?

I hadn't considered that. It sounds like a good idea to investigate.

[patrick-yates-redgate]

Interested to know if there are any plans to progress this further?

@cheenamalhotra / @Wraith2 given your recent contributions have either of you gotten any further with this issue?

[Wraith2]

No. I don't have a way to replicate the problem so i can't investigate further.

[patrick-yates-redgate]

No. I don't have a way to replicate the problem so i can't investigate further.

Thank you for the quick response, much appreciated.

With the original issue #44 being specific to while loops, in relation to this bug do you know if we should be concerned about Cancel causing deadlock for other types of queries?

[cheenamalhotra]

Closing stale PRs, please open a new one when ready with feedback implemented.