Make using scoped services in notification handlers less painful. (#11733)

* Add failing test to demo issue with handlers + scoped dependencies.

* Make using scoped services in notification handlers less painful.

* Update comments for accuracy.

Author: Paul Johnson

src/Umbraco.Core/Events/EventAggregator.Notifications.cs

@@ -7,6 +7,8 @@
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.Extensions.DependencyInjection;
+using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Notifications;
 
 namespace Umbraco.Cms.Core.Events
@@ -84,17 +86,56 @@ public abstract Task HandleAsync(
     internal class NotificationAsyncHandlerWrapperImpl<TNotification> : NotificationAsyncHandlerWrapper
         where TNotification : INotification
     {
+        /// <remarks>
+        /// <para>
+        /// Background - During v9 build we wanted an in-process message bus to facilitate removal of the old static event handlers. <br/>
+        /// Instead of taking a dependency on MediatR we (the community) implemented our own using MediatR as inspiration.
+        /// </para>
+        ///
+        /// <para>
+        /// Some things worth knowing about MediatR.
+        /// <list type="number">
+        /// <item>All handlers are by default registered with transient lifetime, but can easily depend on services with state.</item>
+        /// <item>Both the Mediatr instance and its handler resolver are registered transient and as such it is always possible to depend on scoped services in a handler.</item>
+        /// </list>
+        /// </para>
+        ///
+        /// <para>
+        /// Our EventAggregator started out registered with a transient lifetime but later (before initial release) the registration was changed to singleton, presumably
+        /// because there are a lot of singleton services in Umbraco which like to publish notifications and it's a pain to use scoped services from a singleton.
+        /// <br/>
+        /// The problem with a singleton EventAggregator is it forces handlers to create a service scope and service locate any scoped services
+        /// they wish to make use of e.g. a unit of work (think entity framework DBContext).
+        /// </para>
+        ///
+        /// <para>
+        /// Moving forwards it probably makes more sense to register EventAggregator transient but doing so now would mean an awful lot of service location to avoid breaking changes.
+        /// <br/>
+        /// For now we can do the next best thing which is to create a scope for each published notification, thus enabling the transient handlers to take a dependency on a scoped service.
+        /// </para>
+        ///
+        /// <para>
+        /// Did discuss using HttpContextAccessor/IScopedServiceProvider to enable sharing of scopes when publisher has http context,
+        /// but decided against because it's inconsistent with what happens in background threads and will just cause confusion.
+        /// </para>
+        /// </remarks>
         public override Task HandleAsync(
             INotification notification,
             CancellationToken cancellationToken,
             ServiceFactory serviceFactory,
             Func<IEnumerable<Func<INotification, CancellationToken, Task>>, INotification, CancellationToken, Task> publish)
         {
-            IEnumerable<Func<INotification, CancellationToken, Task>> handlers = serviceFactory
-                .GetInstances<INotificationAsyncHandler<TNotification>>()
+            // Create a new service scope from which to resolve handlers and ensure it's disposed when it goes out of scope.
+            // TODO: go back to using ServiceFactory to resolve
+            IServiceScopeFactory scopeFactory = serviceFactory.GetInstance<IServiceScopeFactory>();
+            using IServiceScope scope = scopeFactory.CreateScope();
+            IServiceProvider container = scope.ServiceProvider;
+
+            IEnumerable<Func<INotification, CancellationToken, Task>> handlers = container
+                .GetServices<INotificationAsyncHandler<TNotification>>()
                 .Select(x => new Func<INotification, CancellationToken, Task>(
                     (theNotification, theToken) =>
-                    x.HandleAsync((TNotification)theNotification, theToken)));
+                        x.HandleAsync((TNotification)theNotification, theToken)));
 
             return publish(handlers, notification, cancellationToken);
         }
@@ -103,13 +144,23 @@ public override Task HandleAsync(
     internal class NotificationHandlerWrapperImpl<TNotification> : NotificationHandlerWrapper
         where TNotification : INotification
     {
+        /// <remarks>
+        /// See remarks on <see cref="NotificationAsyncHandlerWrapperImpl{T}.HandleAsync"/> for explanation on
+        /// what's going on with the IServiceProvider stuff here.
+        /// </remarks>
         public override void Handle(
             INotification notification,
             ServiceFactory serviceFactory,
             Action<IEnumerable<Action<INotification>>, INotification> publish)
         {
-            IEnumerable<Action<INotification>> handlers = serviceFactory
-                .GetInstances<INotificationHandler<TNotification>>()
+            // Create a new service scope from which to resolve handlers and ensure it's disposed when it goes out of scope.
+            // TODO: go back to using ServiceFactory to resolve
+            IServiceScopeFactory scopeFactory = serviceFactory.GetInstance<IServiceScopeFactory>();
+            using IServiceScope scope = scopeFactory.CreateScope();
+            IServiceProvider container = scope.ServiceProvider;
+
+            IEnumerable<Action<INotification>> handlers = container
+                .GetServices<INotificationHandler<TNotification>>()
                 .Select(x => new Action<INotification>(
                     (theNotification) =>
                         x.Handle((TNotification)theNotification)));

tests/Umbraco.Tests.Integration/Implementations/TestHelper.cs

@@ -60,6 +60,11 @@ public TestHelper()
             _ipResolver = new AspNetCoreIpResolver(_httpContextAccessor);
 
             string contentRoot = Assembly.GetExecutingAssembly().GetRootDirectorySafe();
+
+            // The mock for IWebHostEnvironment has caused a good few issues.
+            // We can UseContentRoot, UseWebRoot etc on the host builder instead.
+            // possibly going down rabbit holes though as would need to cleanup all usages of
+            // GetHostingEnvironment & GetWebHostEnvironment.
             var hostEnvironment = new Mock<IWebHostEnvironment>();
 
             // This must be the assembly name for the WebApplicationFactory to work.
@@ -68,6 +73,7 @@ public TestHelper()
             hostEnvironment.Setup(x => x.ContentRootFileProvider).Returns(() => new PhysicalFileProvider(contentRoot));
             hostEnvironment.Setup(x => x.WebRootPath).Returns(() => WorkingDirectory);
             hostEnvironment.Setup(x => x.WebRootFileProvider).Returns(() => new PhysicalFileProvider(WorkingDirectory));
+            hostEnvironment.Setup(x => x.EnvironmentName).Returns("Tests");
 
             // We also need to expose it as the obsolete interface since netcore's WebApplicationFactory casts it.
             hostEnvironment.As<Microsoft.AspNetCore.Hosting.IHostingEnvironment>();

tests/Umbraco.Tests.Integration/TestServerTest/UmbracoTestServerTestBase.cs

@@ -87,7 +87,16 @@ public override IHostBuilder CreateHostBuilder()
 
                  // call startup
                  builder.Configure(app => Configure(app));
-            }).UseEnvironment(Environments.Development);
+            })
+            .UseDefaultServiceProvider(cfg =>
+            {
+                // These default to true *if* WebHostEnvironment.EnvironmentName == Development
+                // When running tests, EnvironmentName used to be null on the mock that we register into services.
+                // Enable opt in for tests so that validation occurs regardless of environment name.
+                // Would be nice to have this on for UmbracoIntegrationTest also but requires a lot more effort to resolve issues.
+                cfg.ValidateOnBuild = true;
+                cfg.ValidateScopes = true;
+            });
 
             return builder;
         }

tests/Umbraco.Tests.Integration/Umbraco.Core/Events/EventAggregatorTests.cs

@@ -0,0 +1,71 @@
+using System;
+using System.Net;
+using System.Net.Http;
+using System.Threading.Tasks;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.DependencyInjection;
+using NUnit.Framework;
+using Umbraco.Cms.Core.Events;
+using Umbraco.Cms.Core.Notifications;
+using Umbraco.Cms.Tests.Integration.TestServerTest;
+
+namespace Umbraco.Cms.Tests.Integration.Umbraco.Core.Events
+{
+    [TestFixture]
+    public class EventAggregatorTests : UmbracoTestServerTestBase
+    {
+        public override void ConfigureServices(IServiceCollection services)
+        {
+            base.ConfigureServices(services);
+            services.AddScoped<EventAggregatorTestScopedService>();
+            services.AddTransient<INotificationHandler<EventAggregatorTestNotification>, EventAggregatorTestNotificationHandler>();
+        }
+
+        [Test]
+        public async Task Publish_HandlerWithScopedDependency_DoesNotThrow()
+        {
+            HttpResponseMessage result = await Client.GetAsync("/test-handler-with-scoped-services");
+            Assert.AreEqual(HttpStatusCode.OK, result.StatusCode);
+        }
+    }
+
+    public class EventAggregatorTestsController : Controller
+    {
+        private readonly IEventAggregator _eventAggregator;
+
+        public EventAggregatorTestsController(IEventAggregator eventAggregator) => _eventAggregator = eventAggregator;
+
+        [HttpGet("test-handler-with-scoped-services")]
+        public async Task<IActionResult> Test()
+        {
+            var notification = new EventAggregatorTestNotification();
+            await _eventAggregator.PublishAsync(notification);
+
+            if (!notification.Mutated)
+            {
+                throw new ApplicationException("Doesn't work");
+            }
+
+            return Ok();
+        }
+    }
+
+    public class EventAggregatorTestScopedService
+    {
+    }
+
+    public class EventAggregatorTestNotification : INotification
+    {
+        public bool Mutated { get; set; }
+    }
+
+    public class EventAggregatorTestNotificationHandler : INotificationHandler<EventAggregatorTestNotification>
+    {
+        private readonly EventAggregatorTestScopedService _scopedService;
+
+        public EventAggregatorTestNotificationHandler(EventAggregatorTestScopedService scopedService) => _scopedService = scopedService;
+
+        // Mutation proves that the handler runs despite depending on scoped service.
+        public void Handle(EventAggregatorTestNotification notification) => notification.Mutated = true;
+    }
+}