1

I've taken over a code base from someone else. This is a web application built on Angular 8 (client) and .NET Core 3.0 (server).

Brief description of the application:

  1. Frequent notifications are stored in a database, with an SqlTableDependency attached to it for detecting new notifications.
  2. When new notifications occur, the server prompts all clients to request an updated list based on their custom filters. These client-to-server requests happen over HttpPost with the filter as a parameter.

The problem occurs when too many notifications arrive at once. Say, when 10 new notifications arrive, the server sends 10 update prompts to the client at the same time, causing the client to immediately send 10 HttpPost requests to the API.

The API takes the filter from the POST, uses it to query the database, and returns the filtered result to the calling client. However, when 10 of these arrive at the same time, it causes a DbContext error - more specific:

A second operation started on this context before a previous operation completed. This is usually caused by different threads using the same instance of DbContext.

public class AlarmController : Controller
{
    private readonly IAlarmRepository alarmRepo;
    private readonly ISiteRepository siteRepo;
    public AlarmController(IAlarmRepository alarmRepo, ISiteRepository siteRepo)
    {
        this.alarmRepo = alarmRepo;
        this.siteRepo = siteRepo;
    }

    [HttpPost("filter")]
    public async Task<IActionResult> FilterAlarm([FromBody] AlarmRequest alarmRequest)
    {
        var snmpReciverList = await this.alarmRepo.GetFilteredSNMPReceiverHistory(alarmRequest.FromDate, alarmRequest.ToDate);
        var siteList = await this.siteRepo.GetSiteListFiltered(int.Parse(alarmRequest.Filter), alarmRequest.SiteName);

        return Ok(await SNMPHistoryMapping.DoMapping(siteList, snmpReciverList);
    }

This HttpPost returns an Ok() with a list of the data requested, in which some mapping is done:

        IEnumerable<Site> sites = siteList;
        IEnumerable<SnmpreceiverHistory> histories = snmpReceiverList;

        IEnumerable<SNMPHistoryResponse> data = (from s in sites
                    join rh in histories on s.Address equals rh.Ipaddress
                    where priority > 0 ? s.SitePriority == priority : true
                    && !string.IsNullOrEmpty(trap) ? rh.AlarmDescription.Contains(trap) : true
                    select new SNMPHistoryResponse()
                    {
                        AlarmDescription = rh.AlarmDescription,
                        EventType = rh.EventType,
                        OnOffStatus = rh.OnOffStatus,
                        ParentSiteName = TraceFullParentDescription(s.Parent),
                        ReceiveTime = rh.ReceiveTime,
                        RepeatCount = rh.RepeatCount,
                        SiteName = s.Description,
                        SitePriority = s.SitePriority,
                        Status = AlarmStatus.GetStatusDescription(rh.EventType),
                        Value = rh.Value
                    });

When multiple of these [HttpPost("filter")] requests arrive at the same time, it appears as if a new thread is created for each one. They all connect on the same DbContext, and the next query starts before the previous is completed.

I can solve it by putting delays between each request from the client, but I want a more robust server-side solution to it, effectively processing these specific requests sequentially.

Note that this is EF Core and .NET Core 3.0, which does not have a SynchronizationContext.

Jonathan Hall
  • 75,165
  • 16
  • 143
  • 189
Scopperloit
  • 920
  • 12
  • 23
  • 1
    *Don't* reuse DbContext. It's just not thread-safe nor does it have to be. It's not a connection, it represents a single unit-of-work. Besides, even with connections, connection pooling means it's cheap to just open and close them as needed. That's why *all* tutorials, examples and production code creates contexts and connections inside `using` blocks. – Panagiotis Kanavos Nov 07 '19 at 18:03
  • I suspect you used the generic repository *anti*pattern to create long-running (singleton?) repositories. That's not how EF, DbContext or even ADO.NET work. In fact, those "generic" repositories put a low-level API on top of high-level abstractions like ORMs and end up eg loading everything in memory when EF and LINQ could load just the columns and entities you wanted – Panagiotis Kanavos Nov 07 '19 at 18:05
  • OK. In this case a single DbContext is created by dependency injection, and then a repository-pattern abstraction is applied on top of that for each of the database tables. In my opinion, this is a pain to debug. I've spent days before pinpointing where this error originated from. – Scopperloit Nov 07 '19 at 18:08
  • `it appears as if a new thread is created for each one.` it doesn't appear - that's what's going on in all web applications. That's expected and well documented. `They all connect on the same DbContext` Don't. DbContext in EF isn't thread-safe. Session in NHibernate isn't thread-safe. They are *not* meant to be reused, they are meant to be disposed/closed as soon as possible. Typically, they are meant to be disposed when a request ends, which is why contexts are registered as Scoped – Panagiotis Kanavos Nov 07 '19 at 18:08
  • 3
    `In this case a single DbContext is created by dependency injection,` don't do that. All examples and tutorials show that the DbContexts are Scoped. `where this error originated from.` it's because of the antipattern. For whatever reason people (I have a specific "framework" in mind) found some 20-year old J2EE books (pre-NHibernate) and blindly copied the patterns there. – Panagiotis Kanavos Nov 07 '19 at 18:09
  • Alright, thanks for good feedback. Looks like I have a lot of work to do. Do you suggest that I scrap the repository-approach that was implemented here? The developers who wrote this have a history of creating rather poor-performance applications, and they're no longer part of our team. That's why this ended up in my hands. – Scopperloit Nov 07 '19 at 18:13
  • 1
    Yes, but. You need to understand what that code does first. *Some* parts, eg stuff like `IEnumerable GetAlll()` are just bad, and end up loading the entire table in memory. You could write a LINQ query using an entity, (the DbSet properties) that execute a single SQL query and return only the data you need. In many SO questions though you'll find people using LINQ queries on top of those buffered results that can't be translated to SQL. – Panagiotis Kanavos Nov 07 '19 at 18:17
  • 1
    A *good* repository would contain methods that simplify and hide complex LINQ queries and returns *only* the data needed by the controllers. – Panagiotis Kanavos Nov 07 '19 at 18:18
  • 2
    I'd argue that there's no such thing as your "good repository". What you're describing is a service class. The repository pattern should not ever be used with an ORM like EF. Full stop. – Chris Pratt Nov 07 '19 at 18:21
  • Good advice, much appreciated. I'm rather fresh in this business, but as you point out, all tutorials I've been through suggest other approaches than what's been done here. Which is also why I'm confused digging through this unrecognizable code. – Scopperloit Nov 07 '19 at 18:23

1 Answers1

2

I believe the comment posted by Panagiotis Kanavos is correct:

In this case a single DbContext is created by dependency injection, don't do that. All examples and tutorials show that the DbContexts are Scoped.

This catches me often, and actually just did. I wasn't using dependency injection, but sharing the DbContext around because I was being lazy. Best to properly set up dependency injection and do it the right way, e.g.:

IHostBuilder host = CreateHostBuilder(args);
host.ConfigureServices(services => {
    services.AddSingleton(service);
    // other stuff...

    // Then the context:
    services.AddScoped<DataContext>(x => {
        DbContextOptionsBuilder<DataContext> dbBuilder =
            new DbContextOptionsBuilder<DataContext>();
        dbBuilder.UseNpgsql(connstr);
        return new DataContext(dbBuilder.Options);
    });
});

// Start the host
host.Build().Run();

The documentation for AddScoped is here, and in true microsoft form, it is impossible to read or digest. Stackoverflow does a better job at explaining it.

David Dombrowsky
  • 1,655
  • 3
  • 20
  • 32