0

I am building a complex .Net Core application with entity framework core. I have multiple db operations in each transaction. I am running an issue with "DataReader already open, and it does not make sense at all as I am using multiple contexts. Here is dummy representations of respective classes

public class MyDbContext : DbContext
{
    private readonly PjSqlConnectionStringBuilder pjSqlConnectionStringBuilder;

    public MyDbContext(PjSqlConnectionStringBuilder pjSqlConnectionStringBuilder):base()
    {
        this.pjSqlConnectionStringBuilder= pjSqlConnectionStringBuilder;
    }
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(this.pjSqlConnectionStringBuilder.ConnectionString);
    }

    public Transaction FetchByTranId(Decimal TranId)
    {
        string query = "TransactionById @TranId;"; // calling a store procedure
        var pId = new SqlParameter("TranId", TranId);
        return this.Transaction.FromSql(query, pId).First();
    }

    public DbSet<Transaction> Transactions { get; set; }
    public DbSet<Sales> DealNos { get; set; }
    public DbSet<DealAuditTrail> DealAuditTrails { get; set; }
    public DbSet<Deal> Deals { get; set; }
    public DbSet<Audit> Audits { get; set; }
}

Then I have two classes that use this contest as follows:

public class TransactionRepository 
{
    public Decimal dealNo;
    private Decimal transactionId;
    public Decimal TransactionId
    {
        get { return this.transactionId; }
        set
        {
            this.transactionId = value;
            Sales d;
            using (var dbContext = new MyDbContext(new PjSqlConnectionStringBuilder()))
            {
                d = dbContext.DealNos.Where(fd => fd.TransactionId == value).First();
            }
            this.dealNo = d.DealNo;
        }
    }

    public Transaction Fetch()
    {
        Transaction t;
        using (var dbContext = new MyDbContext(new PjSqlConnectionStringBuilder()))
        {
            t = dbContext.FetchByTranId(this.transactionId);
        }
        return t;
    }
}

public class AuditRepository
{

    public Task<int> LogRequest(decimal TransactionId, string json)
    {
        var obj = new Audit(TransactionId, "Request", json);
        return this.logObj(obj);
    }

    public Task<int> LogResponse(decimal TransactionId, string json)
    {
        var obj = new Audit(TransactionId, "Response", json);
        return this.logObj(obj);
    }

    private Task<int> logObj(Audit obj)
    {
        using (var dbContext = new MyDbContext(new PjSqlConnectionStringBuilder()))
        {
            dbContext.Audits.Add(obj);
            return dbContext.SaveChangesAsync();
        }

    }
}

The following is the order of executions that causing the error "“There is already an open DataReader associated with this Command which must be closed first.”.

TransactionRepository tr = new TransactionRepository();
tr.TransactionId = 1234;
Transaction  T = tr.Fetch()
.....
.....
AuditRepository ar = new AuditRepository()
var lr1 = ar.LogRequest(tr.TransactionId, T.ToString()) // Exception thrown
....
....

In my understanding, each of the DbContext is separate and not related to each other. Therefore I should not be seeing that error. Any help will be appreciated.

Md Monjur Ul Hasan
  • 1,705
  • 1
  • 13
  • 36
  • For starters, you never wait for `SaveChangesAsync` to complete, so that's an obvious bug that will always fail in production. Second, EF is misused. A DbContext is essentially a Unit-of-Work. You make changes to the entities, and when the time comes you call `await SaveChangesAsync()` or `SaveChanges()` to persist *all* of them. `logObj` on the other hand works as if it was a SqlCommand - no entities, just a single INSERT. Why not use ADO.NET in that case? – Panagiotis Kanavos Feb 13 '20 at 11:51
  • 1
    The overall design of `TransactionRepository` and `AuditRepository` breaks DbContext's behavior too - it no longer acts as a UoW. If you need to discard changes now, you'll have to add explicit database transactions – Panagiotis Kanavos Feb 13 '20 at 11:52
  • Thanks for the suggestion, this are very valuable. But I am still struggle to understand they the contexts are overlapping on each other when they are clearly separate? Any idea? – Md Monjur Ul Hasan Feb 13 '20 at 12:29
  • @Panagiots there is a TaskWait on lr1 in the later part of the code that is not shown Here. However, the exception thrown right at that point and it is not make it any further than that. And the exception does not make any sense as the context is just created. So where is the overlap coming from? – Md Monjur Ul Hasan Feb 13 '20 at 12:31
  • 1
    It doesn't matter where the `await` is if it's *after* the DbContext is disposed. In `logObj`, the DbContext will get disposed even if `SaveChangesAsync` is still running. You're probably getting an exception because the attempt to *close* and reset the connection conflicts with the executing command – Panagiotis Kanavos Feb 13 '20 at 12:33
  • If you want to audit EF operations, there are built-in extension points, like [interceptors](https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-3.0/#interception-of-database-operations) in EF Core 3 or customizing `SaveChagesAsync` in previous versions – Panagiotis Kanavos Feb 13 '20 at 12:37
  • @PanagiotisKanavos Thanks for your comments. That makes sense. Definitely I am a starter on EF. If you put this as an answer, I will be happy to flag it as accepted. – Md Monjur Ul Hasan Feb 13 '20 at 13:20
  • @MdMonjurUlHasan that's basically everything I've already said in the answer below... – Marc Gravell Feb 13 '20 at 14:28

1 Answers1

2

I have a hunch pointing here - this could lead to very odd things:

    private Task<int> logObj(Audit obj)
    {
        using (var dbContext = new MyDbContext(new PjSqlConnectionStringBuilder()))
        {
            dbContext.Audits.Add(obj);
            return dbContext.SaveChangesAsync();
        }
    }

In particular, note that you're disposing a context while an operation is in flow. What you need is to await the pending operation:

    private async Task<int> logObj(Audit obj)
    {
        using (var dbContext = new MyDbContext(new PjSqlConnectionStringBuilder()))
        {
            dbContext.Audits.Add(obj);
            return await dbContext.SaveChangesAsync().ConfigureAwait(false);
        }
    }

Adding the await here ensures that we don't dispose the dbContext until after the save has actually reported completion. The ConfigureAwait is largely optional; there's no need for this code to jump back to the sync-context, so it might as well not bother.

Note that you do not need to do this in LogRequest / LogResponse; they're fine as-written (although I'd probably add the Async suffix onto all 3 methods here). However, your calling code probably should await:

var lr1 = await ar.LogRequest(tr.TransactionId, T.ToString());

and since we're back at the app-tier here, we should let sync-context have a say in what happens (i.e. don't add ConfigureAwait(false) here)

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900