SqlClient Fix managed command cancellation

[Wraith2]

fixes dotnet/SqlClient#109

When using the managed implementation of the sql network interface (unix an uap) any attempt to cancel a request waits until the request completes naturally before the cancellation packet is sent and processed. This results in cancellation taking as long as the query lasts and then acting as if it has been correctly cancelled instead of being cancelled as soon as possible.

This PR modifies the locking used in Send and Receive functions so that packets marked as out-of-band (only attention packets) can bypass the normal lock and be sent immediately. The locking has been changed from a single lock serializing all Send and Receive operations to a pair of locks. Normal in-band packets follow the current behaviour. Out-of-band packets will attempt to take the send-receive lock if possible but continue if it is not. An inner send lock has been added which prevents multiple send operations sending interleaved data, each packet is sent completely.

All tests pass. New tests are added which check that cancellation doesn't just wait for the query to finish.
/cc @afsanehr, @Gary-Zh , @David-Engel , reporter @roji and @saurabh500 from discussion in the issue

[cheenamalhotra]

@Wraith2 This PR is being tracked and will be picked up for dotnet/SqlClient. Appreciate your patience for some more time while we come back to review and merge your contributions! 🙏

[safern]

@cheenamalhotra what's the status of this PR, is there any action we can take here?

[cheenamalhotra]

Hi @safern we will work on these PRs soon, we're occupied with higher priority tasks.

[cheenamalhotra]

default literal not supported in this project.

Suggested change

	DateTime ended = default;
	DateTime ended = DateTime.UtcNow;

[danmoseley]

@Wraith2 can you please commit fix? I don't have permissions to push to your fork.

[Wraith2]

Done, Now isn't the right value, it has to be MInValue.

[stephentoub]

Hi @safern we will work on these PRs soon, we're occupied with higher priority tasks.

@cheenamalhotra, when do you expect to move this forward? If it won't be any time soon, the PR should be closed, with thanks... @Wraith2 has been waiting since June on this.

Should this be closed and opened instead in dotnet/SqlClient?

[Wraith2]

I ported it over to SqlClient, but since it's a bug I left this PR open because I considered it worth fixing.

[cheenamalhotra]

Hi @stephentoub

I intended to get this merged when reviewing but after porting over to Microsoft.Data.SqlClient, I'm not fully confident over this change if this solution is absolutely correct or just a patch with unknown effects, also didn't get too much time to investigate due to other priority tasks.

I don't have too much history of these changes going in right now plus there are some regressions that seem more important and urgent to fix in System.Data.SqlClient as of now.

I would suggest closing this PR for now as I'm hesitant to merge in given time and capacity.
We can backport fix to System.Data.SqlClient later once we stabilize System.Data.SqlClient with other regressions and also confirm solutions in Microsoft.Data.SqlClient.

[stephentoub]

Ok. Thank you. (And thank you @Wraith2.)

[Wraith2]

This change was open for 4 months. If it wasn't the right approach or was incomplete all someone had to do was review it and feedback how to better approach the problem. How is stability going to be improved when a fairly simple change with supporting simple reproduction and supporting tests is just left to rot? This is pathetic and shows the continued lack of intersting and investment in SqlClient.

Seeing the lack of interest in improving it I've tried to contribute but can't do so effectively because there isn't even enough support to review bug fixes. Splitting it into it's own repro seems to be a way to get it out of the main corefx/clr developers hair and somewhere it can be left to stagnate in peace.

[ajcvickers]

@cheenamalhotra @stephentoub Would it be correct to say that this PR has been taken for Microsoft.Data.SqlClient, but is being closed for System.Data.SqlClient? Or is it being rejected for both?

[Wraith2]

There's a version in SqlClient open here dotnet/SqlClient#248

However since moving to M.D.SqlClient is entirely optional and this bug existed in S.D.SqlClient before the other repo was open it made sense to fix it here as well. It should have been simple to get into 3.0 but that missed so my only hope at this point is that it gets into 3.1 so that out of box there was one less bug, as I said #38271 (comment)

There are also the other 3 open PR's that have been in this repo than SqlClient for 6 months now and aren't going anywhere. I've got 2 more perf PR's and the batching feature sat in my local repo waiting for their pre-requisite changes to be ready. In the SqlClient repo perf changes are on hold until the library is stabilized, at the rate things are moving that could take a decade.

[David-Engel]

@ajcvickers It's not being rejected in M.D.SqlClient. It is a fix we would like to get in. There are issues in the two open/active PRs in the SqlClient repo. Until it's sorted out over there, it doesn't feel right to merge this into what is supposed to be the "really stable" SqlClient. Ultimately someone needs to dig into and resolve the test differences/failures on Linux in the PRs.