Error while enumerating query blocks Transaction.Rollback() and swallows exception

[davidbaxterbrowne]

This issue is courtesy of user wired_in at stackoverflow:

https://stackoverflow.com/questions/45971968/rollback-transaction-throws-there-is-already-an-open-datareader-associated-with/45972635#45972635

When encountering an error during object materialization in a transaction the transaction Rollback fails with:

System.InvalidOperationException occurred HResult=0x80131509 Message=There is already an open DataReader associated with this Command which must be closed first.
--

Message=There is already an open DataReader associated with this Command which must be closed first. Source=<Cannot evaluate the exception source> 
StackTrace: at System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command)
--

On EF6 this did not throw an exception.

Here is a minimal repro:

using Microsoft.EntityFrameworkCore;
using System;
using System.Data.SqlClient;
using System.Linq;

namespace ConsoleApp8
{

    public class Foo
    {
        public int Id { get; set; }
    }
    public class Db : DbContext
    {
        public DbSet<Foo> Foos { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Server=localhost;database=EfCoreTest;Integrated Security=true;MultipleActiveResultsets=false");
            base.OnConfiguring(optionsBuilder);
        }

    }
    class Program
    {
        static void Main(string[] args)
        {
            using (var db = new Db())
            {
                db.Database.EnsureDeleted();
                db.Database.EnsureCreated();
                db.Foos.Add(new Foo());
                db.SaveChanges();
            }

            using (var db = new Db())
            {
                var tran = db.Database.BeginTransaction();
                foreach (var foo in db.Foos)
                {
                    tran.Rollback(); // <-- throws
                }

            }

        }
    }
}

The root cause is bad behavior by System.Data.SqlClient.SqlTransaction, and is not limited to .NET core. I don't know if this is a regression or has always behaved like this. SqlTransaction is calling System.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command) before attempting to rollback the transaction. Instead it should probably be sending an Alert to the server (if necessary) and unconditionally rolling back the transaction

EF never directly used SqlTransaction before, as it's actually a pretty bad transaction API for SQL Server. In particular SqlTransaction requires every SqlCommand to be manually enlisted in the transaction, which is not required by SQL Server or TDS. There is at most one active transaction on a SQL Server session, and SQL Server has never allowed queries to "opt out" of the current transaction (this feature is sometimes called "autonomous transactions")

EF6 used EntityTransaction, whereas EF Core appears to use a thin wrapper over the providers DbTransaction implementation.

Here's a repro using just SqlTransaction:

using System;
using System.Data.SqlClient;

namespace ConsoleApp8
{
    class Program
    {
        static void Main(string[] args)
        {
            using (var con = new SqlConnection("Server=localhost;database=tempdb;Integrated Security=true;MultipleActiveResultsets=false"))
            {
                con.Open();
                using (var tran = con.BeginTransaction())
                {
                    var cmd = new SqlCommand("select * from sys.objects", con, tran);
                    var rdr = cmd.ExecuteReader();
                    rdr.Read();

                    tran.Rollback(); //<-- throws
                }
            }
        }
    }
}

Obviously it would be nice to have this fixed in SqlTransaction, now that EF is using it.

Otherwise we could restore EF6 behavior by re-introducing a less-broken DbTransaction implementation built on calling TSQL's BEGIN/COMMIT/ROLLBACK.

[ajcvickers]

@smitpatel In triage, we thought this might be a dupe of an issue that you already fixed. Can you confirm?

[smitpatel]

@ajcvickers - This is not dupe. It is partially related to https://github.com/aspnet/EntityFrameworkCore/pull/9454

If Query fails while enumerating we leak data reader

We fixed that issue. So nomore leaking data reader which could cause this kind of exception. Precisely speaking, if user is running some query in a transaction and wants to rollback transaction if query fails, for which user needs to have explicit try catch block around the query and in catch block, call rollback. It would have not worked earlier because we may not have closed the reader appropriately but it works now.

Though in the both of the repro code above, there is an an active datareader by design. In ADO.NET version, user opened the reader with ExecuteReader method without any using block to manage resources and then without closing reader explicitly calls rollback. So there is an active datareader. Same in case of EFCore, user is in the middle of enumerating a query (which is being streamed instead of evaluated in single call), so we would have active datareader while we are enumerating.

When encountering an error during object materialization in a transaction the transaction Rollback fails with:

This should not be happening as we are not leaking datareader. Though above repro code does not correspond to error during materialization which is being caught by user and calling rollback transaction.

Based on all description above, it seems like that the user wants that whenever Rollback is called, instead of throwing above exception, it should close active datareaders (side-effects unknown). And since EF Core is using SqlTransaction (which EF6 did not), the issue is filed against us to do something about it in EF Core level perhaps.

[ajcvickers]

@divega Thoughts on this?

[mtrainham]

@smitpatel,

This issue was originally raised on stackoverflow, and it has to do with rolling back transactions in the event of an error, using a try/catch block. It makes sense that you would have unintended consequences if you tried to do a rollback in the middle of enumerating a query when no error has occurred. However, what's happening is that when you have a try/catch block (in a global filter in asp.net that wraps the action call for example), and an error happens in the middle of a query, the rollback in the catch is throwing the data reader exception, and is ultimately the error that gets logged, hiding the original error.

This issue doesn't seem to be happening in all cases when an error occurs in the middle of a query. I'm able to reproduce it if I create a mismatch between my EF entity class and my database schema, such as having an nvarchar column mapped to an int property. Then any time I query for that entity, the actual error that is thrown is:

An exception occurred while reading a database value for Property

Then we catch the error, and try to do a rollback, which then throws the datareader error.

You said that the try/catch situation was fixed. In what version is this supposed to be fixed? I have a simple console app that reproduces this issue in EF Core 2.0.

Thanks for looking into this.

[smitpatel]

Confirmed this is fixed by #9454 Due to schema mismatch, query throws InvalidCastException. In 2.0.0 version, this would null out datareader so it was getting leaked. And that would throw at the time of rollback because reader was already open. In latest nightlies, since we do not leak the datareader any longer, it is disposed before rollback is called and the code given in stackoverflow question is working as expected.

[ajcvickers]

Just a note from my discussion with @divega. When using foreach, the enumerator will be disposed when flow leaves the foreach, which in turn will close the data reader in EF. So while there isn't a specific using\finally for the data reader, the use of foreach essentially does the same thing. (Of course, if foreach is not being used, then it is the responsibility of the code doing the enumeration to ensure that the enumerator is disposed, so that would work too if written correctly.)