IDbCommandInterceptor.CommandFailed does not capture all command exceptions
- Dominant language
- C#
- Stars
- 14.8k
- Forks
- 3.4k
- PR merge metrics
- PR metrics pending
Description
## Context
When using EF Core together with Sql Server, we catch exceptions from the database (mostly `SqlException`s) and convert them to domain-specific custom exceptions. Originally we were doing this as a `try`/`catch` inside an override of `SaveChangesAsync` in our `DbContext`.
This approach worked fine for most cases, however when we execute a stored procedure via a call like `DbContext.Database.ExecuteSqlInterpolatedAsync`, it does not use `SaveChangesAsync`, and so exceptions were not being converted in those cases.
In an attempt to create one encapsulated thing that handles the exception handling, I implemented an `IDbCommandInterceptor` using the `CommandFailed` and `CommandFailedAsync` methods.
This appeared to work as we wanted and handled regular calls (made via `SaveChangesAsync`) and stored procedure calls (and other ad-hoc calls) made via `DbContext.Database`.
However, we've come across an edge case where the `IDbCommandInterceptor` approach is not working.
- When changes to multiple entities have been made and then `SaveChangesAsync` is called, EF can decide to make some optimisations. This includes executing a batch of statements in a single database call (which makes sense to do).
- When it does this, it appears that if the first statement of the batch causes an exception, then everything works as expected - the call fails and the exception is passed to the `CommandFailedAsync` of the interceptor.
- However, if the first statement succeeds and returns a result set (such as a new timestamp version column value), but then one of the subsequent statements throws an exception, then the `CommandFailedAsync` of the interceptor is *not* called.
- The end result is still an exception coming from `DbContext.SaveChangesAsync`, it just isn't captured into `CommandFailedAsync`.
After debugging into the code, it appears that this is because:
- the batch statement is being executing as a Db Reader (which makes sense)
- the result `DbDataReader` is only fully processed (via calling `DbDataReader.NextResultAsync`) _after_ the handling for the interceptor is already passed. (`NextResultAsync` is called in `AffectedCountModificationCommandBatch.cs` in the `ConsumeAsync` method)
(Note: I found this blog post helpful in getting to this understanding: https://www.dbdelta.com/the-curious-case-of-undetected-sql-exceptions/)
## My Question
After all the context, my question is:
Is this expected behaviour?
After looking at what the code is doing, it makes sense that in its current form that it behaves this way, but when I was [reading about interceptors](https://learn.microsoft.com/en-us/ef/core/logging-events-diagnostics/interceptors) I had the impression that _all_ command exceptions would be captured into the CommandFailed methods.
If it is intended to work this way, perhaps some additional documentation in the Interceptors page explaining that not all exceptions will be captured would help?
But ideally of course, it would be great if interceptors could be updated/enhanced so that an exception that appears as part of processing the `DbDataReader` would also be caught.
## Issue Reproduction code
As this involves both C# code and Sql Server code and to convince EF that it needs to use its batch command optimisation, this would be quite difficult to create a reproduction for. However, hopefully the explanation above makes sense and is straight-forward to understand.
## Existing Issues Raised
None found on this topic.
## Work-around
In order to get the behaviour we're after, we have changed our code to have handlers in both the interceptor and within our override of `SaveChangesAsync`. This was not a difficult change to make (if a little less neat) but we've only just found out, with a bunch of debugging, that we needed this change and why.
## Alternative Work-around not used
An alternative that was considered was to process the `DbDataReader` within the `ReaderExecutedAsync` similar to what is described within [this issue](https://github.com/dotnet/efcore/issues/31991).
However this involves reading the entire results of every query into memory, which could defeat the purpose of using a reader for large results sets, and it has more risk of inadvertently affecting some other part of how EF works. Another minor point is that `DataTable.Load` does not support async calls, so more custom code would be required to implement that.
## Version information
EF Core version: ``
Target framework: `net8.0`
Contributor guide
Assessment
This issue has not been assessed yet.