HangfireIO / Hangfire

An easy way to perform background job processing in .NET and .NET Core applications. No Windows Service or separate process required
https://www.hangfire.io
Other
9.32k stars 1.69k forks source link

Cannot access a disposed object (LoggerFactory) #2147

Open aleksvujic opened 1 year ago

aleksvujic commented 1 year ago

We are using .NET 6, Hangfire.AspNetCore 1.7.31 and Hangfire.InMemory 0.3.4. This is how we configure Hangfire in Startup.cs:

public void ConfigureServices(IServiceCollection services)
{
  // ...

  services.AddHangfire(configuration =>
  {
    configuration.UseInMemoryStorage();
  });

  services.AddHangfireServer(x =>
  {
    x.WorkerCount = 5;
  });

  // ...
}

We have a unit test which calls RecurringJob.RemoveIfExists("jobId") somewhere in its body. This test was working for a long time and failed randomly today. We re-ran the test multiple times but we were unable to reproduce the exception.

Error message:

System.ObjectDisposedException : Cannot access a disposed object.
Object name: 'LoggerFactory'.

Stacktrace:

at Microsoft.Extensions.Logging.LoggerFactory.CreateLogger(String categoryName)
at Hangfire.AspNetCore.AspNetCoreLogProvider.GetLogger(String name)
at Hangfire.Logging.LogProvider.GetLogger(String name)
at Hangfire.Logging.LogProvider.GetLogger(Type type)
at Hangfire.Client.CoreBackgroundJobFactory..ctor(IStateMachine stateMachine)
at Hangfire.Client.BackgroundJobFactory..ctor(IJobFilterProvider filterProvider)
at Hangfire.RecurringJobManager..ctor(JobStorage storage, IJobFilterProvider filterProvider, ITimeZoneResolver timeZoneResolver, Func`1 nowFactory)
at Hangfire.RecurringJobManager..ctor(JobStorage storage, IJobFilterProvider filterProvider, ITimeZoneResolver timeZoneResolver)
at Hangfire.RecurringJobManager..ctor(JobStorage storage, IJobFilterProvider filterProvider)
at Hangfire.RecurringJobManager..ctor(JobStorage storage)
at Hangfire.RecurringJobManager..ctor()
at Hangfire.RecurringJob.<>c.<.cctor>b__21_0()
at System.Lazy`1.PublicationOnlyViaFactory(LazyHelper initializer)
at System.Lazy`1.CreateValue()
at Hangfire.RecurringJob.RemoveIfExists(String recurringJobId)
at OurApplication.Service.BL.Identity.OurServiceBL.DeleteHangfireRecurringJobIfHangfireIsEnabled(String jobId)

Why did the exception occur? How can we prevent it from happening again?

maxcherednik commented 1 year ago

We are experiencing the same issue since recently.

After looking into the codebase we have a guess that it is related to this line of code: image

What is happening:

  1. We are in the xUnit context
  2. There are several integration tests running in parallel
  3. We are using TestServer from the asp.net core
  4. There are multiple TestServer instance running at the same time
  5. They are stepping on each others toes around that shared static field

There were cases like this before: https://github.com/HangfireIO/Hangfire/issues/1099 I don't think it was actually fixed.

odinserj commented 1 year ago

For the original question, this exception is thrown likely because ASP.NET Core host is shutting down. And in this case we should understand why this happens, and especially why we are executing business logic after application is shutting down, because in this case it is expected that nothing will be called.

Regarding unit tests, Hangfire for historical reasons uses static fields for its configuration and perhaps the best way is to avoid running such tests in parallel.

maxcherednik commented 1 year ago
  1. Within 1 test nothing is been executed after the server is disposed.
  2. We can not run those test cases sequentially. It does not make any sense for us.

Something like this is happening: image

As a workaround, I had to disable the hangfire Hosted service from starting in the tests.

Potential solution: Since LibLog is discontinued, maybe it is time to take the dependency on Microsoft.Extensions.Logging

odinserj commented 1 year ago

With serial tests you'll be able to call LogProvider.SetCurrentLogProvider before each test runs, so that each test will have a fresh logging provider instance. If you are using non-static BackgroundJobClient or RecurringJobManager classes instead of static Background/RecurringJob that you'll have no static logger references.

Or alternatively you can just use NoOpLogProvider to avoid using logger from .NET Core that throws exceptions after being disposed.

There are no problems with LibLog, exception is arising from Microsoft's log provider that was disposed.

maxcherednik commented 1 year ago

There are no problems with LibLog, exception is arising from Microsoft's log provider that was disposed.

It is not a problem, it is a design limitation of the LibLog. It sets a static variable. The tests are in the common process space. Hence, overlapping.

Microsoft's log provider that was disposed.

That is correct. I asked it to. That is what is happening when the test case is over. But the reference(the wrong one by the way) to the log provider got stuck in the static field.

odinserj commented 1 year ago

Static configuration unfortunately is the problem for Hangfire itself, not only for LibLog.

maxcherednik commented 1 year ago

Any plans to replace the LibLog with the Microsoft.Extensions.Logging?

mmigala commented 10 months ago

Having the same issue. I also disabled Hangfire from running in Integration tests for now...

Since we are using .NET 6 and NUnit with WebApplicationFactory for integration tests I've overridden implementation of Hangfire interface that does nothing.

Posting since maybe it will be useful for someone.

public class CustomWebApplicationFactory : WebApplicationFactory<Program>
{
    protected override void ConfigureWebHost(IWebHostBuilder builder)
    {
        builder.ConfigureServices(OverrideExistingHangfireInterfaceImplementation);
    }

    private static void OverrideExistingHangfireInterfaceImplementation(IServiceCollection services)
    {
        // Find the existing registration and remove it
        var descriptor = services.SingleOrDefault(
            d => d.ServiceType ==
                 typeof(IHangfireRecurringJobManager));
        if (descriptor != null)
        {
            services.Remove(descriptor);
        }

        // Add a new registration
        services.AddTransient<IHangfireRecurringJobManager, StubHangfireRecurringJobManager>();
    }
}

public class StubHangfireRecurringJobManager : IHangfireRecurringJobManager
{
    public void AddOrUpdate(string recurringJobId, Expression<Action> methodCall, string cronExpression)
    {
    }

    public void AddOrUpdate(string recurringJobId, Expression<Func<Task>> methodCall, string cronExpression)
    {
    }

    public void RemoveIfExists(string recurringJobId)
    {
    }
}
rossknudsen commented 3 months ago

I was experiencing an issue with using AutomaticRetryAttribute in my unit tests that produced the same issue as described by OP. I found that in my case, I could provide an alternative implementation to the built in one, as a workaround:

internal class NoRetryAttribute : JobFilterAttribute, IElectStateFilter
{
    public void OnStateElection(ElectStateContext context)
    {
        // No-op overrides the default retry behaviour.
    }
}

I then use it in my unit test setup like:

// For testing purposes, we will not perform any retries.
GlobalConfiguration.Configuration.UseFilter(new NoRetryAttribute());

Note: I'm using AspNetCore, .Net 8.0 and implementing Integration tests along the lines of the MS recommendations.