I was working with SignalR, and created timer that will execute following code where I need to get the number of unread messages from database

[Route("api/[controller]")]
[ApiController]
public class PorukeHubController : ControllerBase
{
    private readonly IHubContext<PorukeHub> _hub;
    private readonly TimerManager _timer;
    private DnevnikContext _context { get; set; }

    public PorukeHubController(IHubContext<PorukeHub> hub,
        TimerManager timer, DnevnikContext context)
    {
        _hub = hub;
        _timer = timer;
        _context = context;
    }

    [HttpGet]
    public IActionResult Get()
    {
        var currentUserId = 1;

        if (!_timer.IsTimerStarted)
            _timer.PrepareTimer(() =>
            {

                var unreadMessages = _context.Messages
                    .Where(p => p.RecieverID == currentUserId && p.isRead == false)
                    .Count();

                _hub.Clients.All.SendAsync("checkForMessages", unreadMessages);
            });
        return Ok(new { Message = "Request Completed" });
    }

Unfortunately, I get the following error when trying to access _context:

System.ObjectDisposedException: '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. ObjectDisposed_ObjectName_Name'

I'm very confused on what steps should I take to solve this, I'm not yet familiar that much with DI too Any help would be appreciated

CodePudding user response：

You should turn your method into an async method and await the database call.

public async Task<IActionResult> Get()
{
   ...
   var unreadMessages = await _context.Messages.Where(p => p.RecieverID == currentUserId && p.isRead == false).Count();
   ...
}

CodePudding user response：

A DbContext only lives for a short period of time and its lifetime ends at the end of the web request for which it is created.

The delegate, however, that you use to initialize the TimerManager stores a reference to that DbContext likely for the duration of the application (assuming that TimerManager is a Singleton). But since that DbContext is disposed of soon after you initialized the TimerManager it becomes unusable.

In general, you should prevent moving injected dependencies from thread to thread (except in case those threads are part of a sequentially executed asynchronous operation), because the consuming code (i.e. your controller) doesn't know whether or not it is safe to do so.

This means that, instead of reusing the same DbContext, the timer should get its own instance and, preferable, get a fresh DbContext instance every time the timer is triggered. You should see each timer tick as a new request, just like every call to a controller is a new (web) request. And the general rule of thumb is that each new request gets its own container scope.

What this means is that, instead of reusing the same DbContext instance, you should wrap each tick in a container scope / IServiceScope and resolve the DbContext from that scope.

For instance:

private readonly TimerManager _timer;
private readonly IServiceProvider _provider;

public PorukeHubController(TimerManager timer, IServiceProvider provider)
{
    _timer = timer;
    _provider = provider;
}

[HttpGet]
public IActionResult Get()
{
    var currentUserId = 1;

    if (!_timer.IsTimerStarted)
        _timer.PrepareTimer(() =>
        {
            using (var scope = this.provider.CreateScope())
            {
                var sp = scope.ServiceProvider;
                var context = sp.GetRequiredService<DnevnikContext>();
                var hub = sp.GetRequiredService<IHubContext<PorukeHub>>();
                
                var unreadMessages = context.Messages
                    .Where(p => p.RecieverID == currentUserId && p.isRead == false)
                    .Count();

                hub.Clients.All.SendAsync("checkForMessages", unreadMessages);
            }
        });
        
    return Ok(new { Message = "Request Completed" });
}

Although the code above fixes the initial problem of letting the DbContext go out of scope, it poses a new problem, which is more fundamental in nature, which is that application components shouldn't depend on the (IServiceProvider) DI Container. This is called the Service Locator anti-pattern.

This means that you shouldn't do this type of initialization inside your controllers, but instead move this to the application startup path; a.k.a. the Composition Root.

This can be done, for instance, by introducing a new abstraction:

private readonly IMessageInitializer _initializer;

public PorukeHubController(IMessageInitializer initializer)
{
    __initializer = _initializer;
}

[HttpGet]
public IActionResult Get()
{
    _messageInitializer.Initialize();

    return Ok(new { Message = "Request Completed" });
}

In this code example the new IMessageInitializer hides the complexity of initialization of the timer, the querying of the database and the calling of the hub from the controller.

Inside your Composition Root you can now define an implementation with the original code:

public class ScheduledMessageInitializer : IMessageInitializer
{
    private readonly TimerManager _timer;
    private readonly IServiceProvider _provider;

    public ScheduledMessageInitializer(
        TimerManager timer, IServiceProvider provider)
    {
        _timer = timer;
        _provider = provider;
    }

    public void Initialize()
    {
        var currentUserId = 1;

        if (!_timer.IsTimerStarted)
            _timer.PrepareTimer(() => {
                using (var scope = this.provider.CreateScope())
                {
                    var sp = scope.ServiceProvider;
                    var context = sp.GetRequiredService<DnevnikContext>();
                    var hub = sp.GetRequiredService<IHubContext<PorukeHub>>();
                    
                    var unreadMessages = context.Messages
                        .Where(p => p.RecieverID == currentUserId && p.isRead == false)
                        .Count();

                    hub.Clients.All.SendAsync("checkForMessages", unreadMessages);
                }
            });
    }
}

This class can be registered as follows:

// This assumes that TimerManager is a singleton as well.
services.AddSingleton<IMessageInitializer, ScheduledMessageInitializer>();

This still poses a (smaller) design issue, which is that with DI you should strive to keep the application's business logic out of the Composition Root. It should only contain the required infrastructural code for the application to execute. The querying of the database and sending it to the hub can be considered business logic; not infrastructure.

That would mean that one last refactoring is in place: you should extract that logic out of the ScheduledMessageInitializer and place it in a new application component:

public class UnreadMessageChecker
{
    private readonly DnevnikContext _context;
    private readonly IHubContext<PorukeHub> _hub;

    public UnreadMessageChecker(DnevnikContext context, IHubContext<PorukeHub> hub)
    {
        _context = context;
        _hub = hub;
    }
    
    public async Task CheckAsync()
    {
        var unreadMessages = context.Messages
            .Where(p => p.RecieverID == currentUserId && p.isRead == false)
            .Count();

        // I noticed how you called SendAsync. You should await it, otherwise
        // you'll get into the same trouble as where you started out with:
        // with an ObjectDisposedExcetion.
        await hub.Clients.All.SendAsync("checkForMessages", unreadMessages);
    }
}

services.AddTransient<UnreadMessageChecker>();

This new component can be resolved from the ScheduledMessageInitializer, which reduces it to the following:

public class ScheduledMessageInitializer : IMessageInitializer
{
    private readonly TimerManager _timer;
    private readonly IServiceProvider _provider;

    public ScheduledMessageInitializer(
        TimerManager timer, IServiceProvider provider)
    {
        _timer = timer;
        _provider = provider;
    }

    public void Initialize()
    {
        var currentUserId = 1;

        if (!_timer.IsTimerStarted)
            _timer.PrepareTimer(async () =>
            {
                using (var scope = this.provider.CreateScope())
                {
                    var checker = scope.ServiceProvider
                        .GetRequiredService<UnreadMessageChecker>();

                    await checker.CheckAsync();
                }
            });
    }
}

There might still be other issues with your code. For instance, it seems weird to me that you have a currentUserId (which is runtime data, changing on each request), while using it to initialize the timer with; unless that timer isn't a Singleton. But if the timer isn't a singleton, that would mean that would be initializing an endless number of timers, which likely isn't a good idea as well.

Another issue is that, if the TimerManager is indeed singleton, there might be a race condition while initializing. Is the TimerManager thread-safe? What would happen when it gets initialized twice simultaneously? Would that cause problems. I, unfortunately, can't answer this.