0

I found the equivalent of the following in the software I work on:

using(var dbContext = Startup.Services.BuildServiceProvider().GetService<ApplicationDbContext>())
{
    while(true)
    {
        var results = async dbContext.GetDataAsync();
        if(!results.Any()
            break;
        foreach(var result in results){
            [...do stuff...]
        }
        await dbContext.SaveChangesAsync();

        // Give the DB time to catch-up
        // NOTE: The DB struggles with not having connections available, so let other tasks
        //       have a chance at connecting to the DB.
        await Task.Delay(TimeSpan.FromSeconds(10));
    }
}

We have been having some performance issues as we scale up our operations, particularly with running out of available database connections. Having a while(true) loop inside a using(dbContext) block doesn't seem like a very good idea, but it does eventually run out of results and hits the break on line 7, so its not really an infinite loop.

What really concerns me is the last line. As the comments (copied from the source code) show, the idea is to essentially throttle this operation so it doesn't clog up the database connection pool. Pretty sure GetDataAsync and SaveChangesAsync will each grab a new connection from the pool, so this is probably working as intended.

I know Task.Delay releases the active thread to allow other traffic through, but it must be keeping the dbContext in memory the entire time and reusing it in each loop. From what I know, it is generally better to use a new dbContext each time.

I'm sure there are a number of ways we could optimize this, but that is not my immediate concern. What I need to figure out is whether this code is actually something to be concerned about in regards to the performance issues we've been seeing. Could this be having any significantly negative effects on performance? Or is it more a matter of not being optimal?

WillRoss1
  • 123
  • 4
  • See [this answer](https://softwareengineering.stackexchange.com/a/359672/115084) – John Wu May 18 '21 at 22:30
  • _"From what I know, it is generally better to use a new dbContext each time"_ -- that is not the guidance given for that class. See duplicate. – Peter Duniho May 18 '21 at 22:37
  • 2
    John Wu's link is helpful and germane. The "duplicate" listed is not. There is a potential for problems here, depending on how your DbContext is constructed, and if I'm reading this correctly as a queue processing task you probably are getting a memory leak by making the context track the state of objects that you're not planning to use again after SaveChangesAsync is called. But this particular piece of code probably isn't what's causing you to run out of connections. – StriplingWarrior May 18 '21 at 22:43

0 Answers0