From 78f5b2f0d7bd9e7c76b5b4949e36d43a4ccc9354 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 9 Mar 2021 12:45:51 +1100 Subject: [PATCH] removes double httpcontextaccessor registration and updates test notes. --- .../Services/ThreadSafetyServiceTest.cs | 45 +++++++++++++++++-- .../UmbracoBuilderExtensions.cs | 7 ++- 2 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ThreadSafetyServiceTest.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ThreadSafetyServiceTest.cs index 557035a1e0..5611c7cb07 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ThreadSafetyServiceTest.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ThreadSafetyServiceTest.cs @@ -2,6 +2,7 @@ // See LICENSE for more details. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; using System.Threading; @@ -14,6 +15,7 @@ using Umbraco.Cms.Core.Services.Implement; using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; +using Umbraco.Extensions; using Constants = Umbraco.Cms.Core.Constants; namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services @@ -120,12 +122,27 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services { try { - log.LogInformation("[{0}] Running...", Thread.CurrentThread.ManagedThreadId); + ConcurrentStack currentStack = ((ScopeProvider)ScopeProvider).GetCallContextScopeValue(); + log.LogInformation("[{ThreadId}] Current Stack? {CurrentStack}", Thread.CurrentThread.ManagedThreadId, currentStack?.Count); + + // NOTE: This is NULL because we have supressed the execution context flow. + // If we don't do that we will get various exceptions because we're trying to run concurrent threads + // against an ambient context which cannot be done due to the rules of scope creation and completion. + // But this works in v8 without the supression!? Why? + // In v8 the value of the AmbientScope is simply the current CallContext (i.e. AsyncLocal) Value which + // is not a mutable Stack like we are maintaining now. This means that for each child thread + // in v8, that thread will see it's own CallContext Scope value that it set and not the 'true' + // ambient Scope like we do now. + // So although the test passes in v8, there's actually some strange things occuring because Scopes + // are being created and disposed concurrently and out of order. + var currentScope = (Scope)ScopeAccessor.AmbientScope; + log.LogInformation("[{ThreadId}] Current Scope? {CurrentScope}", Thread.CurrentThread.ManagedThreadId, currentScope?.GetDebugInfo()); + Assert.IsNull(currentScope); string name1 = "test-" + Guid.NewGuid(); IContent content1 = contentService.Create(name1, -1, "umbTextpage"); - log.LogInformation("[{0}] Saving content #1.", Thread.CurrentThread.ManagedThreadId); + log.LogInformation("[{ThreadId}] Saving content #1.", Thread.CurrentThread.ManagedThreadId); Save(contentService, content1); Thread.Sleep(100); // quick pause for maximum overlap! @@ -133,11 +150,12 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services string name2 = "test-" + Guid.NewGuid(); IContent content2 = contentService.Create(name2, -1, "umbTextpage"); - log.LogInformation("[{0}] Saving content #2.", Thread.CurrentThread.ManagedThreadId); + log.LogInformation("[{ThreadId}] Saving content #2.", Thread.CurrentThread.ManagedThreadId); Save(contentService, content2); } catch (Exception e) { + //throw; lock (exceptions) { exceptions.Add(e); @@ -147,6 +165,8 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services threads.Add(t); } + // See NOTE above, we must supress flow here to be able to run concurrent threads, + // else the AsyncLocal value from this current context will flow to the child threads. using (ExecutionContext.SuppressFlow()) { // start all threads @@ -199,7 +219,22 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services { try { - log.LogInformation("[{0}] Running...", Thread.CurrentThread.ManagedThreadId); + ConcurrentStack currentStack = ((ScopeProvider)ScopeProvider).GetCallContextScopeValue(); + log.LogInformation("[{ThreadId}] Current Stack? {CurrentStack}", Thread.CurrentThread.ManagedThreadId, currentStack?.Count); + + // NOTE: This is NULL because we have supressed the execution context flow. + // If we don't do that we will get various exceptions because we're trying to run concurrent threads + // against an ambient context which cannot be done due to the rules of scope creation and completion. + // But this works in v8 without the supression!? Why? + // In v8 the value of the AmbientScope is simply the current CallContext (i.e. AsyncLocal) Value which + // is not a mutable Stack like we are maintaining now. This means that for each child thread + // in v8, that thread will see it's own CallContext Scope value that it set and not the 'true' + // ambient Scope like we do now. + // So although the test passes in v8, there's actually some strange things occuring because Scopes + // are being created and disposed concurrently and out of order. + var currentScope = (Scope)ScopeAccessor.AmbientScope; + log.LogInformation("[{ThreadId}] Current Scope? {CurrentScope}", Thread.CurrentThread.ManagedThreadId, currentScope?.GetDebugInfo()); + Assert.IsNull(currentScope); string name1 = "test-" + Guid.NewGuid(); IMedia media1 = mediaService.CreateMedia(name1, -1, Constants.Conventions.MediaTypes.Folder); @@ -224,6 +259,8 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services threads.Add(t); } + // See NOTE above, we must supress flow here to be able to run concurrent threads, + // else the AsyncLocal value from this current context will flow to the child threads. using (ExecutionContext.SuppressFlow()) { // start all threads diff --git a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs index cd2b930b20..0126d443c9 100644 --- a/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs @@ -92,9 +92,9 @@ namespace Umbraco.Extensions services.AddLogger(tempHostingEnvironment, loggingConfig, config); - // TODO: This doesn't seem right? The HttpContextAccessor is normally added to the container - // with ASP.NET Core's own ext methods. Is there a chance we can end up with a different - // accessor registered and resolved? + // Manually create and register the HttpContextAccessor. In theory this should not be registered + // again by the user but if that is the case it's not the end of the world since HttpContextAccessor + // is just based on AsyncLocal, see https://github.com/dotnet/aspnetcore/blob/main/src/Http/Http/src/HttpContextAccessor.cs IHttpContextAccessor httpContextAccessor = new HttpContextAccessor(); services.AddSingleton(httpContextAccessor); @@ -242,7 +242,6 @@ namespace Umbraco.Extensions builder.Services.AddUmbracoImageSharp(builder.Config); // AspNetCore specific services - builder.Services.AddUnique(); builder.Services.AddUnique(); builder.AddNotificationHandler();