SaveChangesAsync should not wrap OperationCancellationException with DbUpdateException

[maxcherednik]

Cancellation contract of async methods is broken.

If a method: SaveChangesAsync(Boolean, CancellationToken) is called and then cancelled, I would expect to catch an 'OperationCanceledException'. Instead, it throws DbUpdateException with the TaskCanceledException as an inner exception.

According to the documentation it was designed to be so. But is it a good design decision?

Microsoft.EntityFrameworkCore.DbUpdateException: An error occurred while updating the entries. See the inner exception for details. ---> System.Data.SqlClient.SqlException: A severe error occurred on the current command.  The results, if any, should be discarded.
Operation cancelled by user.
   at System.Data.SqlClient.SqlCommand.<>c.<ExecuteDbDataReaderAsync>b__180_0(Task`1 result)
   at System.Threading.Tasks.ContinuationResultTaskFromResultTask`2.InnerInvoke()
   at System.Threading.Tasks.Task.Execute()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.<ExecuteAsync>d__17.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.<ExecuteAsync>d__32.MoveNext()
   --- End of inner exception stack trace ---
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.<ExecuteAsync>d__32.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.<ExecuteAsync>d__10.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.<ExecuteImplementationAsync>d__31`2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.<ExecuteImplementationAsync>d__31`2.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<SaveChangesAsync>d__79.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<SaveChangesAsync>d__77.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.EntityFrameworkCore.DbContext.<SaveChangesAsync>d__52.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at DbContext`2.<SaveChangesAsync>d__8.MoveNext()

[roji]

Note the same question at the ADO.NET/SqlClient level: https://github.com/dotnet/corefx/issues/9245. I do agree that in general cancellations should be raised as OperationCanceledExceptions rather than domain-specific DbUpdateException/DbException/whatever.

[roji]

Notes:

• SaveChangesAsync does throw TaskCanceledException when passed a canceled token - DbUpdateException is only thrown when the cancellation occurs later.
• Query and ExecuteSqlRawAsync simply bubble up the original ADO.NET provider exception, whatever that is. In SqlClient's case that's SqlException which is bad, but that's Cancelling an async SqlClient operation throws SqlException, not TaskCanceledException SqlClient#26
• Cosmos only wraps CosmosException with a DbUpdateException, as opposed to Relational which wraps everything. So it already bubbles up OperationCanceledException, but we may want to consider wrapping other exceptions as well, /cc @AndriySvyryd.

[AndriySvyryd]

but we may want to consider wrapping other exceptions as well

Makes sense

[roji]

Opened #25631 to track wrapping other exception types for Cosmos

[roji]

@AndriySvyryd @ajcvickers we may want to document this as a low-impact breaking change - if anyone's catching DbUpdateException with the expectation that cancellations are included there, that code would no longer work.

[janseris]

Hi, I am afraid that this issue is still there.
When inserting items into database and calling SaveChangesAsync with cancellation token and the token is activated by user to cancel the request, SqlException with Class 11 is fired instead of OperationCancelledException.

This is also seen in this comment which shows how to properly catch the token cancellation:

dotnet/SqlClient#26 (comment)

I see it tracked also here: dotnet/SqlClient#26 but there is no resolution of this issue.

Are there any plans for this small breaking change or do you suggest any solution, for example using this DbCommandInterceptor?
dotnet/SqlClient#26 (comment) which looks nice but might have some unexpected drawbacks?

Thank you

[roji]

@janseris this issue wasn't meant to fix or work around dotnet/SqlClient#26; it was only about not wrapping OperationCancelledException with DbUpdateException. This helps for other providers which do properly throw OperationCancelledException, but doesn't affect SqlClient which doesn't.

However, note that in 7.0 we also did #19526, which is about identifying cancellation exceptions and logging a different message, to allow users to e.g. not log for cancellations. Since there's (apparently) no way to identify cancellation exceptions with SqlClient, the approach there was to look at the cancellation token, and if it's triggered, treat the exception as a cancellation. We could use the same approach and wrap the SqlException in an OperationCancelledException, fixing the SqlClient behavior at the EF level.

@janseris can you please open a new issue for us to discuss this?