SQL.OperationCancelled Returns SqlException Instead of OperationCancelledException

[thefringeninja]

https://github.com/dotnet/corefx/blob/master/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlUtil.cs#L391

This is making cooperative cancellation difficult.

[roji]

See also https://github.com/dotnet/corefx/issues/9245, where apparently SqlException sometimes gets thrown.

[karinazhou]

Hi,
I will look into this. Thank you for pointing this out.

[karinazhou]

It looks like this exception throwing logic has existed in SqlClient for a long time.
@thefringeninja could you please explain a bit more why this is making cooperative cancellation difficult?

[roji]

@karinazhou the issue is that OperationCancelledException is treated in a special way. A Task that aborts with that exception is marked as cancelled (as opposed to fail), and this distinction can be quite important.

One thing I'm wondering about, though: this specific exception may be thrown as a result of DbCommand.Cancel() - the older sync cancellation API - rather than the newer async API which uses cancellation tokens. If so, that would at least explain how this came about.

[thefringeninja]

@karinazhou This particular InvalidOperationException has a culture specific message, so catching it with a pattern match will be extremely difficult.

[karinazhou]

@roji @thefringeninja Thank you for the explanation and I can understand the difficulties you mention when handling InvalidOperationException for cancellation scenario.

After looking a bit more into code, this SQL.OperationCancelled() is mainly called by SQLCommand and TdsParser. As what @roji suspects, the SQLCommand does derive from DbCommand and probably that is one of the reasons why InvalidOperationException is used here to keep the old way. I do see some cancellation token usage in System.Data.Sqlclient for Async mode though.

One thing we are concerning about is the backward compatibility of different exception types. This will definitely impact those who have expected InvalidOperationException when the operation is cancelled by the user.

[divega]

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.

[roji]

@karinazhou I think it may be acceptable for DbCommand.Cancel() to continue throwing InvalidOperationException - IMHO the main point is for the async, cancellation-token-based cancellation to raise OperationCancelledException - that is where Tasks are involved and handling of the different exception types is especially important.

However, I did a quick test to get a full picture of the situation, and had some surprising results:

1. When doing sync cancellation (SqlCommand.Cancel()), the command runs to completion (no interruption, cancellation is ignored), but an SqlException is still thrown at the end. Note that I'm running on Linux, this may be a problem (Ubuntu 19.04, .NET Core SDK 3.0.100-preview5-011568). We should run the same test on Windows to be sure, and probably open another issue.
2. Async cancellation interrupts immediately as expected, and throws SqlException instead of the desirable OperationCancelledException. https://github.com/dotnet/corefx/issues/9245 already tracks this.

In other words, it's not clear in which actual situation InvalidOperationException is thrown - @thefringeninja did you encounter this in an actual runtime scenario, or see this in the codebase?

Code used for the above test:

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

[thefringeninja]

My mistake @roji, after looking more closely at our build logs it is indeed SqlException: https://travis-ci.org/SQLStreamStore/SQLStreamStore/builds/531673632#L1294 Not sure where I got InvalidOperationException from! I will fix the original issue.

[roji]

@thefringeninja in that case this issue is a dup of https://github.com/dotnet/corefx/issues/9245 and can be closed.

@karinazhou it may still be worth investigating who actually calls OperationCancelled() in the code base, will leave this issue open for now. We also need to see about the failure to actually interrupt the query in sync mode (possibly a Linux-specific issue).

[thefringeninja]

Closing in favor of #26

[karinazhou]

@roji Thanks for the testing code snippet. We will investigate more.

[roji]

FYI the issue with cancellation not working does not repro on Windows (thanks @divega), only on Linux. Opened #109 to track that.