0

Using Task.Run causes "object disposed" exception in my application if using DbContext.

The code looks like (see the whole chain):

UserController.cs

[Route("api/[controller]")]
public class UsersController : ControllerBase
{
    private readonly UserService userService;

    /// <summary>
    /// </summary>
    /// <param name="userService"></param>
    public UsersController(UserService userService)
    {
        this.userService = userService;
    }


    public async Task<ActionResult> DoSomething()
    {
        await this.userService.MyMethod();
        return this.Ok();
    }
}

UserService.cs

public class UserService
{
    private readonly UserRepository userRepository;

    public UserService(UserRepository userRepository)
    {
        this.userRepository = userRepository;
    }
    
    public async Task MyMethod()
    {
        // some logic
        Task.Run(() => MethodCallAsync());
    }

    void MethodCallAsync()
    {
        // some logic
        // calls UserRepository, which uses the DbContext by DI
    }   
}

UserRepository:

public class UserRepository
{
    private MyDbContext dbContext;

    public UserRepository(MyDbContext dbContext)
    {
        this.dbContext = dbContext;
    }

    public async Task DoSomethingToo(string username)
    {
        var user = await this.dbContext.Users.SingleOrDefaultAsync(u => u.Username == username);
        // some logic
    }
}

Causes the following exception (message):

Cannot access a disposed context instance. A common cause of this error is disposing a context instance that was resolved from dependency injection and then later trying to use the same context instance elsewhere in your application. This may occur if you are calling 'Dispose' on the context instance, or wrapping it in a using statement. If you are using dependency injection, you should let the dependency injection container take care of disposing context instances.

How i did configure my db context and UserRepository:

Startup.cs:

    public void ConfigureServices(IServiceCollection services)
    {
        // some logic
        if (MyDbContextFactory.GetConnectionString() != null)
        {
            services.AddDbContext<MyDbContext>(options => options.UseMySQL(MyDbContextFactory.GetConnectionString())
            .LogTo(s => System.Diagnostics.Debug.WriteLine(s)));
        }
        
        services.AddScoped(typeof(UserService));
        services.AddScoped(typeof(UserRepository));
        // some logic
    }
    
    public void Configure(IApplicationBuilder app, IHostingEnvironment env)
    {
        // some logic
        using (var serviceScope = app.ApplicationServices.CreateScope())
        {
            var dbContext = serviceScope.ServiceProvider.GetService<MyDbContext>();

            if (dbContext != null)
            {
                dbContext.Database.Migrate();
            }
        }
        // some logic
    }
    
    public class MysqlEntityFrameworkDesignTimeServices : IDesignTimeServices
    {
        public void ConfigureDesignTimeServices(IServiceCollection serviceCollection)
        {
            serviceCollection.AddEntityFrameworkMySQL();
            new EntityFrameworkRelationalDesignServicesBuilder(serviceCollection)
                .TryAddCoreServices();
        }
    }

MyDbContextFactory.cs

    public MyDbContext CreateDbContext(string[] args)
    {
        var optionsBuilder = new DbContextOptionsBuilder<MyDbContext>();
        optionsBuilder.UseMySQL(GetConnectionString());
        return new MyDbContext(optionsBuilder.Options);
    }

If i replace Task.Run with BackgroundJob.Enqueue it works fine. But hangfire creates a lot (> 1k) of entries in redis within some minutes, since this method is called very often. Besides that, if it works with hangfire, it should also work with Task.Run,

Jimy Weiss
  • 568
  • 1
  • 9
  • 25
  • Can you please provide more code from the routine where you're using the `DbContext`? Of particular interest is how you create it or otherwise where are you getting it from. – Sergey Kudriavtsev Mar 07 '23 at 10:51
  • Most likely the issues you have are coming from using incorrect scope. `DbContext` is designed to be used in "scoped" scope, with "create-use once-dispose" usage pattern. – Sergey Kudriavtsev Mar 07 '23 at 10:53
  • @SergeyKudriavtsev i did now also proivde the UserRepository which is using the DbContext and how i register UserRepository in Startup.cs. Do you need more information? – Jimy Weiss Mar 07 '23 at 10:55
  • 2
    See [this answer](https://stackoverflow.com/a/75655314/2501279) – Guru Stron Mar 07 '23 at 10:55
  • @JimyWeiss ...and how do you get the `UserRepository` in `MethodCallAsync`? – Sergey Kudriavtsev Mar 07 '23 at 10:57
  • @SergeyKudriavtsev now you can see the whole chain of calls and UserRepository and UserService are registered as scoped services. – Jimy Weiss Mar 07 '23 at 11:09
  • 2
    Remove all attempts to "improve" EF Core to begin with. A DbContext isn't a database connection or low-level driver, that's what ADO.NET is. It's already a multi-entity Repository and disconnected Unit-Of-Work. Since it's a Unit-of-Work, it's not meant to be thread-safe. There's no point when all changes are persisted at once in a single transaction when `SaveChanges` is called. That unawaited `Task.Run(() => MethodCallAsync());` is a fire-and-forget task that may not even start running before the request ends and all DbContexts and services are disposed – Panagiotis Kanavos Mar 07 '23 at 11:28
  • 1
    If, and only if, you find a valid reason to raise the abstraction level above EF Core, add the necessary classes. Given how high-level EF Core already is, that typically means creating a high-level domain repository that only loads entire object graphs. The point at this level is to allow you to eg switch from relational databases to Mongo, not MySQL to PostgreSQL (that's what EF does) – Panagiotis Kanavos Mar 07 '23 at 11:29
  • As for testing and mocking, a mock must be at the same or higher level than the class it mocks. If you really want to abstract a DbContext, you need to mock the domain repo, not the entities *below* the DbContext – Panagiotis Kanavos Mar 07 '23 at 11:30
  • @PanagiotisKanavos what do you mean with "attempts to improve EF Core"? Do you have some concrete suggestions? – Jimy Weiss Mar 07 '23 at 11:54

2 Answers2

4

The core problem is that you now have code that runs outside the scope of a request (i.e., request-extrinsic code). You want to return early and then update the database after the user receives the response (or more accurately, after the response is sent).

Part of tearing down that request scope is disposing any instances that are scoped to that request, including DbContext and friends.

Besides that, if it works with hangfire, it should also work with Task.Run

No.

The entire point of Hangfire is to provide reliable request-extrinsic code. Task.Run does not do this; it just throws work onto the thread pool. If, for example, you want to do a rolling update, then your app will be shut down once all its responses are sent, and work just tossed onto the thread pool may be lost. Hangfire prevents work loss by using a durable queue; this is usually a database but it sounds like yours is set up to use Redis (note that Redis is not durable by default, and it should be configured to be durable if you're using it as a backend for Hangfire).

Since MethodCallAsync accesses the database, I interpret it as work you don't want to ever lose. If that's correct, then your options are:

  1. Call await MethodCallAsync directly without using Hangfire or Task.Run. This increases your response time but gives you one nice guarantee: if your client receives a successful response, then it knows the work completed successfully.
  2. Use Hangfire (with a proper durable queue) or build your own basic distributed architecture (as explained on my blog). This allows you to return early and then do the work later. Note that the work is done in its own scope, independent of the request scope. There is also some complexity dealing with failing work; the usual approach is to use a dead-letter queue and set up an alerting system so you can fix it manually.

Note that Task.Run is not a valid option, because it's work you can't lose, and Task.Run can lose work.

But hangfire creates a lot (> 1k) of entries in redis within some minutes, since this method is called very often.

It sounds like your actual problem is that Hangfire isn't very efficient, which is true. First, I'd recommend segregating the Hangfire data from any application data; e.g., if your app also uses the same Redis instance, then move Hangfire's data to a different Redis instance. Then scale up if necessary. If you still can't get it efficient enough, then I recommend using your own durable queue (e.g., Azure Storage Queues / Amazon SQS / Google Cloud Tasks / Kafka / RabbitMQ if configured to be durable / etc) instead of Hangfire.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
1

Your problem essentially is that Task.Run(() => MethodCallAsync()); starts a separate thread that is still relying on the class field userRepository (and DbContext inside it) - this is inherently not thread-safe and is not supposed to be by design. One way you can fix this is by passing a higher-level instance into your async method - e.g. replacing userRepository not just with DbContext itself, but rather with IDbContextFactory<DbContext>. This is about how it would look like:

public class UserService
{
    private readonly IDbContextFactory<MyDbContext> dbContextFactory;

    public UserService(IDbContextFactory<MyDbContext> dbContextFactory)
    {
        this.dbContextFactory = dbContextFactory;
    }
    
    public async Task MyMethod()
    {
        // some logic
        Task.Run(() => MethodCallAsync());
    }

    void MethodCallAsync()
    {
        // some logic
        var userRepository = new UserRepository(dbContextFactory);
        ...
    }   
}

public class UserRepository
{
    private MyDbContext dbContext;

    public UserRepository(IDbContextFactory<MyDbContext> dbContextFactory)
    {
        this.dbContext = dbContextFactory.CreateDbContext();
    }

    public async Task DoSomethingToo(string username)
    {
        var user = await this.dbContext.Users.SingleOrDefaultAsync(u => u.Username == username);
        // some logic
    }
}

DbContextFactory is supposed to be thread-safe, so this would probably work. However, if we dive deeper into your code, I would join Panagiotis Kanavos and say that you likely don't need the UserRepository class at all. DbContext is supposed to be your Repository and Unit of Work already, so why not use it directly?

public class UserService
{
    private readonly IDbContextFactory<MyDbContext> dbContextFactory;

    public UserService(IDbContextFactory<MyDbContext> dbContextFactory)
    {
        this.dbContextFactory = dbContextFactory;
    }
    
    public async Task MyMethod()
    {
        // some logic
        Task.Run(() => MethodCallAsync());
    }

    void MethodCallAsync()
    {
        // some logic
        using var dbContext = dbContextFactory.CreateDbContext();
        var user = await dbContext.Users.SingleOrDefaultAsync(u => u.Username == username);
        ...
    }   
}

You will have two times less code for the same thing and it will be working in a thread-safe manner.

Sergey Kudriavtsev
  • 10,328
  • 4
  • 43
  • 68
  • No i get the following exception: System.InvalidOperationException: Unable to resolve service for type 'Microsoft.EntityFrameworkCore.IDbContextFactory`1[Access.Database.MyContext]' while attempting to activate 'Web.Service.UserService – Jimy Weiss Mar 07 '23 at 13:33
  • @JimyWeiss Apologies, you also need to register it properly: https://learn.microsoft.com/en-us/ef/core/dbcontext-configuration/#using-a-dbcontext-factory-eg-for-blazor – Sergey Kudriavtsev Mar 07 '23 at 13:42
  • 1
    now it works with dbContextFactory. – Jimy Weiss Mar 07 '23 at 20:44