BeginTransaction with ReadUncommitted can cause dirty reads for out-of-transaction queries

[samcic]

Consider the following HomeController, modified from a fresh Asp.Net Core 2.0 MVC template project in Visual Studio 2017 (15.6.6):

public class HomeController : Controller
    {
        private readonly ApplicationDbContext _applicationDbContext;
        
        public HomeController(ApplicationDbContext applicationDbContext)
        {
            _applicationDbContext = applicationDbContext;
        }

        /// <summary>
        /// Add and save a user in a read-uncommitted transaction, but don't commit the changes until some time has passed
        /// </summary>
        /// <returns></returns>
        public async Task<ContentResult> CreateUser1()
        {
            var username = $"{AppConstants.UserPrefix}[email protected]";
            var executionStrategy = _applicationDbContext.Database.CreateExecutionStrategy();
            await executionStrategy.ExecuteAsync(async () =>
            {
                using (var t =
                    await _applicationDbContext.Database.BeginTransactionAsync(System.Data.IsolationLevel.ReadUncommitted))
                {
                    _applicationDbContext.Users.Add(new ApplicationUser { UserName = username, NormalizedUserName = username, Email = username, NormalizedEmail = username });
                    await _applicationDbContext.SaveChangesAsync();
                    await Task.Delay(TimeSpan.FromSeconds(120)); // Simulate other work on which transaction committing depends
                    t.Commit();
                }
            });
            return Content("ok");
        }

        /// <summary>
        /// Add and save a user in a read-uncommitted transaction, then commit the transaction WITHOUT DELAY
        /// </summary>
        /// <returns></returns>
        public async Task<ContentResult> CreateUser2()
        {
            var username = $"{AppConstants.UserPrefix}[email protected]";
            var executionStrategy = _applicationDbContext.Database.CreateExecutionStrategy();
            await executionStrategy.ExecuteAsync(async () =>
            {
                using (var t =
                    await _applicationDbContext.Database.BeginTransactionAsync(System.Data.IsolationLevel.ReadUncommitted))
                {
                    _applicationDbContext.Users.Add(new ApplicationUser { UserName = username, NormalizedUserName = username, Email = username, NormalizedEmail = username });
                    await _applicationDbContext.SaveChangesAsync();
                    t.Commit();
                }
            });
            return Content("ok");
        }

        /// <summary>
        /// Add and save a user in a read-committed transaction
        /// </summary>
        /// <returns></returns>
        public async Task<ContentResult> CreateUser3()
        {
            var username = $"{AppConstants.UserPrefix}[email protected]";
            _applicationDbContext.Users.Add(new ApplicationUser { UserName = username, NormalizedUserName = username, Email = username, NormalizedEmail = username });
            await _applicationDbContext.SaveChangesAsync();
            return Content("ok");
        }

        /// <summary>
        /// Asserts the current transaction isolation level is read-committed
        /// </summary>
        /// <returns></returns>
        public async Task<ContentResult> AssertReadCommitted()
        {
            try
            {
                await _applicationDbContext.Database.ExecuteSqlCommandAsync("IF (select transaction_isolation_level from sys.dm_exec_sessions where session_id = @@SPID)=2 select 'ok'; ELSE THROW 51000, 'NOT READ COMMITTED ISOLATION!.', 1; ");
                return Content("ok");
            }
            catch (Exception e)
            {
                return Content(e.Message);
            }
        }

        /// <summary>
        /// Attempt to get user1 using a standard EF Core query.
        /// </summary>
        /// <returns></returns>
        public async Task<ContentResult> GetUser1()
        {
            var username = $"{AppConstants.UserPrefix}[email protected]";
            var user = await _applicationDbContext.Users.Where(u => u.NormalizedUserName == username).FirstOrDefaultAsync();
            return Content("Result: " + user);
        }
    }

    public static class AppConstants
    {
        public static readonly string UserPrefix = Guid.NewGuid().ToString("N").ToUpperInvariant();
    }

Steps to Reproduce the problem

1. Run the solution and navigate to /home/createuser1 in a browser (this will spin because of the delay waiting to commit)

2. Run all of the following steps while the above operation is still waiting to complete ("spinning")

• Open another tab in the browser and navigate to /home/getuser1. This will return the expected empty result (because user1 hasn't yet been committed from /home/createuser1). Note also that navigating to /home/AssertReadCommitted will return "ok" (i.e. read-committed isolation for normal queries).

• Open another tab in the browser and navigate to /home/createuser2. This will create another user immediately in a read-uncommitted transaction

• Open another tab in the browser and navigate to /home/getuser1. This now returns the user1 record, which is unexpected because it still hasn't been committed. This implies that there has been a dirty read (I'm proposing that this is an issue). Note that navigating to /home/AssertReadCommitted will now also return an exception string showing that we have read-uncommitted isolation for normal queries.

• Open another tab in the browser and navigate to /home/createuser3. This will create another user immediately in a read-committed transaction

• Open another tab in the browser and navigate to /home/getuser1. Now the user1 record is not returned any more, because we're back to read-committed behavior after the creation of user3. Note also that navigating to /home/AssertReadCommitted will return "ok" (i.e. read-committed isolation for normal queries).

Remarks

I might be missing something, but it seems to me that creating a transaction with BeginTransactionAsync and ReadUncommitted should mean that only the queries within that transaction are affected by the isolation level. Each separate web request gets a different ApplicationDbContext instance if I'm not mistaken, but presumably they all use the same underlying "connection"/"session" to the database, hence the propagation of the transaction isolation level change to other web requests.

My particular use case: I'm writing a booking application that involves payment. When someone creates a booking, I need to make sure that 1) there are no conflicts in time with other bookings, including other new bookings for which the payment might currently be being processed, and 2) the payment goes through successfully. My approach was going to be something along the lines of:

            var executionStrategy = _applicationDbContext.Database.CreateExecutionStrategy();
            await executionStrategy.ExecuteAsync(async () =>
            {
                using (var t =
                    await _applicationDbContext.Database.BeginTransactionAsync(System.Data.IsolationLevel.ReadUncommitted))
                {
                    // 1. Insert and Save the booking
                    // 2. Retrieve all potential conflicts (including uncommitted ones from this operation in other transactions)
                    // 3. If conflict, rollback, otherwise process payment
                   // 4. If payment fails, rollback, otherwise commit transaction
                }
            });

Given the above issue, I couldn't use this approach because it would mean that other normal GET/read queries might seeing uncommitted booking data (that may e.g. still fail payment), which of course I want to avoid.

I'd be grateful for any comments on whether or not this is indeed a bug, and the recommended approach for solving such scenarios.

Further technical details

EF Core version: Microsoft.AspNetCore.All 2.0.7, Microsoft.EntityFrameworkCore.Tools 2.0.2
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 Pro 10.0.16299
IDE: Visual Studio 2017 15.6.6
Corresponding VS Solution ZIP: TransactionsDemo.zip

[pmiddleton]

What version of Sql Server are you running? Prior to Sql 2014 transaction isolation levels can leak across pooled connections.

[samcic]

SQL Server 14.0.1000.169

[samcic]

Would it be helpful if I reproduce this on Azure SQL?

[ajcvickers]

/cc @divega

[ajcvickers]

Closing old issue as this is no longer something we intend to implement.

[Trolldemorted]

@ajcvickers so this is not a bug but working as intended?

[ajcvickers]

@dotnet/efcore Can someone provide more details on the reason for this being closed as won't-fix--I don't remember enough of the discussion in triage.

[samcic]

Just for reference, we were a little disappointed to hear that this wasn't considered something worth resolving. We've since implemented a messy hack to help achieve what we want in our app (see remarks in the original post), but from our perspective it'd be much cleaner if ReadUncommitted were supported.

Regardless of the above, the scope "leakage" behavior seems to be cause for some concern...it has the potential to cause significant side-effects on completely unrelated requests. From this perspective at least I feel that this needs some attention (even if it's dropping support for ReadUncommitted usage completely to protect against the leakage problem).

[roji]

Isn't this another occurrence of isolation level leak across pooled connections, i.e. dotnet/SqlClient#96 (which seems to still be an issue, regardless of SQL Server version)?

[ajcvickers]

@roji To take another look.

[roji]

I took another look at the scenario above, and it does seem like the root cause is dotnet/SqlClient#96, i.e. SqlClient leaking isolation levels across pooled connections.

• In step 2, /home/createuser2 is accessed. This gets a (new) connection and executes an operation with ReadUncommitted. The connection is then returned to the pool.
• In step 3, /home/getuser1 is accessed. This presumably gets the previous connection from the pool, which has retained the ReadUncommitted isolation level (this is precisely the bug SqlTransaction and TransactionScope leak isolation level SqlClient#96 describes). Because of this, the uncommitted user1 record is returned, even if its inserting transaction hasn't committed yet.

So this doesn't seem to be related to EF Core. Until dotnet/SqlClient#96 is resolved, any application which executes a transaction in the non-default isolation level should probably reset the level back (e.g. by executing a second dummy transaction).

[roji]

@samcic you can test the above by running your same flow with pooling off - if I'm right, then it should work as expected. It would be good to get a confirmation for that.

[samcic]

@roji Thanks for looking into this. I can confirm that putting Pooling=False in the connection string resolves the problem. Given this, we may use the same workaround suggested in dotnet/SqlClient#96, i.e. using a different application name to create two different connection strings that then results in two different pools (one specifically only for opt-in ReadUncommitted isolation level connections).

[roji]

@samcic thanks for confirming. A possibly lighter workaround would be to manually reset your isolation level back to ReadCommitted after switching to ReadUncommitted (or any other isolation level), by doing a no-op transaction. This would add a roundtrip, but fragmenting your connection pool via a different application name may be even more problematic.

[samcic]

@roji Thanks very much, but I'm a little confused about how that would look exactly. Might you be kind enough to share the corresponding change to my original code sample above so I can see what you mean?

I'm namely trying to understand why there wouldn't be race conditions with other requests grabbing the ReadUncommitted connection from the pool in the small amount of time after I'm finished with my ReadUncommitted work but before I'm able to do the no-op.

I'm also trying to understand what would happen if there were to be a transient issue (e.g. database server temporarily unavailable) which would mean the no-op couldn't be executed and the connection perhaps still left in ReadUncommitted state in the pool.

[roji]

@samcic you're right that it's a bit tricky, since EF Core automatically opens and closes connections between operations. However, you can explicitly open the connection yourself, and perform all the work plus the no-op transaction before closing it. The underlying DbConnection should never get returned to the pool until you close it, allowing you to to reset its isolation level.