umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.42k stars 2.67k forks source link

Scoped services are disposed when used in Async Notification handlers #14574

Closed cf-marc closed 1 year ago

cf-marc commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

12.0.1

Bug summary

When the Application tries to execute the EF Database-Migration an exception is thrown.

Specifics

No response

Steps to reproduce

Follow the Instruction of the tutorial: https://docs.umbraco.com/umbraco-cms/tutorials/getting-started-with-entity-framework-core

After registering the notification handler, build the project and start the Application.

My NotificationHandler looks like this:

using System.Runtime.InteropServices.ComTypes;
using Microsoft.EntityFrameworkCore;
using Serilog.Context;
using Statistics.Service.Data.Contexts;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Persistence.EFCore.Scoping;

namespace Statistics.Service.Notifications
{
    public class RunBlogCommentsMigration : INotificationAsyncHandler<UmbracoApplicationStartedNotification>
    {
        private readonly BlogContext _blogContext;

        public RunBlogCommentsMigration(BlogContext blogContext)
        {
            _blogContext = blogContext;
        }

        public async Task HandleAsync(UmbracoApplicationStartedNotification notification, CancellationToken cancellationToken)
        {
            IEnumerable<string> pendingMigrations = await _blogContext.Database.GetPendingMigrationsAsync();

            if (pendingMigrations.Any())
            {
                await _blogContext.Database.MigrateAsync();
            }
        }
    }
}

Expected result / actual result

expected result: The Migration is executed and the Database is created.

actual result:

System.AggregateException: One or more errors occurred. (One or more errors occurred. (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.
Object name: 'BlogContext'.))
 ---> System.AggregateException: One or more errors occurred. (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.
Object name: 'BlogContext'.)
 ---> 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.
Object name: 'BlogContext'.
   at Microsoft.EntityFrameworkCore.DbContext.CheckDisposed()
   at Microsoft.EntityFrameworkCore.DbContext.get_Database()
   at Statistics.Service.Notifications.RunBlogCommentsMigration.HandleAsync(UmbracoApplicationStartedNotification notification, CancellationToken cancellationToken) in C:\Projects\EHCB\EHCB-Website\CMS\Statistics.Service\Notifications\RunBlogCommentsMigration.cs:line 26
   at Umbraco.Cms.Core.Events.INotificationAsyncHandler`1.HandleAsync(IEnumerable`1 notifications, CancellationToken cancellationToken)
   at Umbraco.Cms.Core.Events.EventAggregator.PublishCoreAsync[TNotification](IEnumerable`1 allHandlers, IEnumerable`1 notifications, CancellationToken cancellationToken)
   at Umbraco.Cms.Core.Events.EventAggregator.PublishNotificationsAsync[TNotification,TNotificationHandler](IEnumerable`1 notifications, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Threading.Tasks.Task.WaitAllCore(Task[] tasks, Int32 millisecondsTimeout, CancellationToken cancellationToken)
   at System.Threading.Tasks.Task.WaitAll(Task[] tasks)
   at Umbraco.Cms.Core.Events.EventAggregator.Publish[TNotification,TNotificationHandler](IEnumerable`1 notifications)
   at Umbraco.Cms.Core.Events.EventAggregator.Publish[TNotification](TNotification notification)
   at Umbraco.Cms.Infrastructure.Runtime.CoreRuntime.<StartAsync>b__22_1()
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location ---
   at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException)
   --- End of inner exception stack trace ---
   at System.Threading.CancellationTokenSource.ExecuteCallbackHandlers(Boolean throwOnFirstException)
   at Microsoft.Extensions.Hosting.Internal.ApplicationLifetime.NotifyStarted()

_This item has been added to our backlog AB#31413_

github-actions[bot] commented 1 year ago

Hi there @cf-marc!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

Zeegaan commented 1 year ago

I sadly cannot reproduce this 😢 Here is the steps i did:

namespace efcore_test.EFCore;

public class RunBlogCommentsMigration : INotificationAsyncHandler { private readonly BlogContext _blogContext;

public RunBlogCommentsMigration(BlogContext blogContext)
{
    _blogContext = blogContext;
}

public async Task HandleAsync(UmbracoApplicationStartedNotification notification, CancellationToken cancellationToken)
{
    IEnumerable<string> pendingMigrations = await _blogContext.Database.GetPendingMigrationsAsync();

    if (pendingMigrations.Any())
    {
        await _blogContext.Database.MigrateAsync();
    }
}

}

public class BlogCommentsComposer : IComposer { public void Compose(IUmbracoBuilder builder) => builder.AddNotificationAsyncHandler<UmbracoApplicationStartedNotification, RunBlogCommentsMigration>(); }



Am I doing something different here ? 🤔 
cf-marc commented 1 year ago

Hey @Zeegaan

Thanks for your reply. I noticed the difference and didn't recognize that you are using another provider, sorry for that. I am using "Microsoft.Data.SqlClient" as provider. It works for me when I use synchronous calls for both methods.

Zeegaan commented 1 year ago

Ah yes, that did the trick, weird one indeed! I can reproduce this 🐛

AleksandrRogov commented 1 year ago

I was getting the same error for the same provider and also followed the same tutorial.

Also I did some investigation where I replaced the line

builder.Services.AddUmbracoEFCoreContext<BlogContext>(connectionString, "Microsoft.Data.SqlClient");

with out of the box EF Core db context registration

builder.Services.AddDbContext<BlogContext>(options => options.UseSqlServer(connectionString));

just to eliminate a possibility if there is something wrong with Umbraco's code.

It still failed, but with a different error: "BeginExecuteNonQuery requires an open and available Connection. The connection's current state is closed." So, I added _blogContext.Database.OpenConnection(); to the Notification Handler, like so:

public class RunBlogCommentsMigration : INotificationAsyncHandler<UmbracoApplicationStartedNotification>
{
    private readonly BlogContext _blogContext;

    public RunBlogCommentsMigration (BlogContext blogContext)
    {
        _blogContext= blogContext;
    }

    public async Task HandleAsync(UmbracoApplicationStartedNotification notification, CancellationToken cancellationToken)
    {
        try
        {
            _blogContext.Database.OpenConnection(); //<--- here
            //next line worked and I could get pending migrations
            IEnumerable<string> pendingMigrations = await _blogContext.Database.GetPendingMigrationsAsync();

            if (pendingMigrations.Any())
            {
                //but then failed with "Cannot access a disposed context instance" here
                await _blogContext.Database.MigrateAsync();
            }
        }
        catch(Exception ex)
        {
            throw;
        }
    }
}

It worked and I could get pending migrations, but then it failed later when trying to call MigrateAsync. Looks like there is an error that is happening somewhere else that disposes the context instance.

I also tried to use an async method await _blogContext.Database.OpenConnectionAsync(); - it failed on trying to retrieve pending migrations with "Cannot access a disposed context instance" error.

So then I went ahead and replaced an asynchronous Umbraco Notification Handler with a synchronous one and this time the migrations worked fine.


I also have another asynchronous Notification Handler on "ContentPublishingNotification" and there is a code that accesses my custom EF database context which also fails with "Cannot access a disposed context instance" error. When I try to re-use that code (it's basically a repository class that uses a DbContext to get the data) anywhere else, for example, in an MVC Controller - it works perfectly fine.

Bear with me, there is more... 😃

After all of that, I replaced the EF Core DbContext registration code back to the one that Umbraco has: builder.Services.AddUmbracoEFCoreContext. And added Umbraco scope provider IEFCoreScopeProvider to my "repository" class that accesses DbContext to get the data and enclosed EF CRUD code inside the scope, similar to an example from the tutorial:

using IEfCoreScope<BlogContext> scope = _efCoreScopeProvider.CreateScope();

await scope.ExecuteWithContextAsync<Task>(async db =>
{
        db.BlogComments.Add(comment);
        await db.SaveChangesAsync();
});

scope.Complete();

I then tested my asynchronous ContentPublishingNotification handler with a modified code and got the following error:

System.InvalidOperationException, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e: The Scope 047563da-f7fe-45c0-a959-e356a5d6cd23 being disposed is not the Ambient Scope af838e1a-2a96-437b-8121-16bc70eb246f. This typically indicates that a child Scope was not disposed, or flowed to a child thread that was not awaited, or concurrent threads are accessing the same Scope (Ambient context) which is not supported. If using Task.Run (or similar) as a fire and forget tasks or to run threads in parallel you must suppress execution context flow with ExecutionContext.SuppressFlow() and ExecutionContext.RestoreFlow().

Stack Trace:

at Umbraco.Cms.Infrastructure.Scoping.Scope.Dispose()
   at Umbraco.Cms.Core.Services.ContentService.SaveAndPublish(IContent content, String culture, Int32 userId)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PublishInternal(ContentItemSave contentItem, String defaultCulture, String cultureForInvariantErrors, Boolean& wasCancelled, String[]& successfulCultures)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSaveInternal[TVariant](ContentItemSave contentItem, Func`3 saveMethod, Func`2 mapToDisplay)
   at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.PostSave(ContentItemSave contentItem)
   at lambda_method1400(Closure, Object)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.AwaitableObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)

These are all my findings so far, but I hope they will help to identify the root of the problem. The bottom line is that there is something strange going on with EF Core DbContext (with SqlServer provider) when used inside Umbraco's asynchronous notification handlers.

Hope this helps. Let me know if more details needed.

bergmania commented 1 year ago

Just playing around with this.. Seems to work as expected if I use the EF Core scope provider from my notification, like this

public class RunBlogCommentsMigration : INotificationAsyncHandler<UmbracoApplicationStartedNotification>
{
    private readonly IEFCoreScopeProvider<BlogContext> _efCoreScopeProvider;

    public RunBlogCommentsMigration(IEFCoreScopeProvider<BlogContext> efCoreScopeProvider)
    {
        _efCoreScopeProvider = efCoreScopeProvider;
    }

    public async Task HandleAsync(UmbracoApplicationStartedNotification notification, CancellationToken cancellationToken)
    {
        using IEfCoreScope<BlogContext> scope = _efCoreScopeProvider.CreateScope();

        await scope.ExecuteWithContextAsync<Task>(async db =>
        {
            IEnumerable<string> pendingMigrations = await db.Database.GetPendingMigrationsAsync(cancellationToken: cancellationToken);

            if (pendingMigrations.Any())
            {
                await db.Database.MigrateAsync(cancellationToken: cancellationToken);
            }
        });

        scope.Complete();
    }
}

But I will try to understand what is going on better

bergmania commented 1 year ago

Fix cherry picked to release/12.1 + v10/dev.

The issue was because the scoped services - in this case the BlogContext - was disposed in the async notification handler, because the event aggregator did return a task without awaiting it.