SqlCommand.Cancel() does not work on Linux

[roji]

SqlCommand.Cancel() doesn't appear to work using System.Data.SqlClient on Linux (Ubuntu 19.04, .NET Core SDK 3.0.100-preview5-011568) - the command runs to completion, and only then throws SqlException. The issue does not seem to repro on Windows (with .NET Core).

Repro:

using (var cmd = new SqlCommand("WAITFOR DELAY '00:00:05'", conn))
{
    // sync cancellation
    Task.Delay(1000).ContinueWith(t => cmd.Cancel());
    var sw = Stopwatch.StartNew();
    try
    {
        cmd.ExecuteNonQuery();
    }
    catch (Exception e)
    {
        Console.WriteLine($"Sync cancellation: caught {e.GetType().Name} after {sw.ElapsedMilliseconds}ms");
        // Does not interrupt the call (completes after 5 seconds) but still generates SqlException at the end
    }

    // async cancellation
    sw.Restart();
    try
    {
        await cmd.ExecuteNonQueryAsync(new CancellationTokenSource(1000).Token);
    }
    catch (Exception e)
    {
        Console.WriteLine($"Async cancellation: caught {e.GetType().Name} after {sw.ElapsedMilliseconds}ms");
        // Interrupts and generates SqlException immediately
    }
}

/cc @divega @karinazhou

[karinazhou]

Thank you for repro steps. We will investigate more on this one.

[David-Engel]

Triage: This would be nice to fix for 1.0, but may not rise above other GA tasks at this point.

[roji]

FWIW it may be worth confirming whether this somehow stems from a more low-level issue (i.e. some sort of corefx networking issue) as opposed to an SqlClient-issue.

[Wraith2]

It's a managed implementation issue. I've reproduced it on windows using your test script above and have added a new test.

There are lock(this) statements in the managed send and receive methods on SniTCPHandle. So what happens is you start a query which ultimately tracks down into Receive which takes the lock and then from another thread you call Cancel which tracks down into SendAttention and that tries to Send and blocks on the lock. When the receive times out the lock is released the send works, the attention is processed and everything unravels in the correct order just a lot later that you want it to.

I think send and receive aren't mutually exclusive but that each one is not concurrent with itself. Changing the lock to be operation specific consumes a bit of object space but there shouldn't be too many tcp handles around since they're reused. It will need some careful testing because locking in this library is definitely "here be dragons" territory.

[roji]

@Wraith2 your explanation makes perfect sense, and I can see how allowing concurrent send/receive is not to be done lightly... :)

[saurabh500]

My 2 c. Send and receive are mutually exclusive. All sends are also mutually exclusive except for the Attention packet.
Good catch @Wraith2
Cancel ATTN packet is the only packet in the protocol which can be sent out of band.

[saurabh500]

This is a bug, and I can understand based on @Wraith2 explanation why this is happening. What is the fix?

[Wraith2]

Based on your assertions i think the fix is to change the Send implementation to require the lock unless the packet is detectably OOB (ATTN is the only one of these i know of)) from the packet header.

[saurabh500]

Uploading a snippet from the TDS protocol to make this clearer.

[saurabh500]

I would say, the client should try to send the attention packet ASAP, by inserting it into the TDS stream, as soon as it finishes writing the current packet.

All sends are also mutually exclusive except for the Attention packet.

To refine this further, all requests are send independently of each other except for ATTN packet which can be interleaved in between other packets for an ongoing request.

[Wraith2]

So i think my proposal is to have two locks and these rules.

1. all in-band send and receive operations MUST aquire the in-band lock (currently Monitor.Enter(this))
2. out-of-band communications MAY aquire (would be Monitor.TryEnter(this, out aquired) and detect attn packet header flag)
3. all send operations MUST aquire the send lock (new _sendLock, lock(_sendLock))

[roji]

Just be careful you don't end up in a state where you're writing an out-of-band cancellation, without having acquired the lock, just as a user send starts - with the two interfering with one another. I'm not sure of the cross-OS guarantees with regards to atomicity of a single write (i.e. is a read write guaranteed to complete without interference even if another write is in progress).

FYI in PostgreSQL the protocol obviates this kind of problem by making clients send cancellations on a totally separate TCP connection (although that sometimes creates its own difficulties).