Efcore: Savepoints should not be be rolled back/released if the connection is closed

[roji]

Our SQL Server ExecutionStrategyTests are incomplete because TestRelationalTransaction mistakenly exposes AreSavepointsSupported=false.

Once this is fixed, tests in ExecutionStrategyTest fail because of ExecuteNonQuery requires an open and available Connection. The connection's current state is closed.. This is because BatchExecutor attempts to roll back to the savepoint on exception, but if the ADO connection has been closed because of the exception this triggers an error. The same does not occur for regular transaction rollback, because disposing the transaction always completes without an exception, even if the connection is closed.

Options for dealing with this:

• Check if the ADO connection is still open before attempting to roll back to the savepoint (or release it). Note that the RelationalConnection is unaware that its ADO connection is closed; should we detect this situation and propagate the closing to the RelationalConnection?
• Swallow any exceptions coming out of the rollback/release. This uses exceptions for flow control which isn't ideal.

As an aside, I'm wondering if we should look into a better API for System.Data (https://github.com/dotnet/runtime/issues/33397). A CreateSavepoint API could return a disposable, providing similar semantics to transaction itself. Specifically, disposing it would automatically rollback and never produce an exception (at the price of an extra allocation for the disposable). EF Core would still have to support older providers so this is a bit orthogonal.

PS We should also consider promoting ExecutionStrategyTest to a derivable test suite rather than making it a SQL Server only thing.

[AndriySvyryd]

We should do both: Check whether the ADO connection is still open (RelationalConnection status shouldn't always mirror the underlying connection, it's more like the status that EF expects the connection to be). And also swallow exceptions from rollback/release if we are already in a faulty state as the exception from this might make the ExecutionStrategy not retry.

[roji]

BTW swallowing is important even if we check the connection, because an unrelated exception may be raised from rollback/release even if the connection is completely fine at the moment when we check it (and we want to bubble the original exception).