yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

removes double httpcontextaccessor registration and updates test notes.

Browse Source
This commit is contained in:
Shannon
2021-03-09 12:45:51 +11:00
parent a07728f8db
commit 78f5b2f0d7
2 changed files with 44 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/ThreadSafetyServiceTest.cs
View File

@@ -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<IScope> 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<IScope> 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

7
src/Umbraco.Web.Common/DependencyInjection/UmbracoBuilderExtensions.cs
View File

@@ -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<IHttpContextAccessor, HttpContextAccessor>();
builder.Services.AddUnique<IRequestAccessor, AspNetCoreRequestAccessor>();
builder.AddNotificationHandler<UmbracoRequestBegin, AspNetCoreRequestAccessor>();