diff --git a/src/Umbraco.Core/FireAndForgetTasks.cs b/src/Umbraco.Core/FireAndForgetTasks.cs deleted file mode 100644 index 5cd8b5cb2d..0000000000 --- a/src/Umbraco.Core/FireAndForgetTasks.cs +++ /dev/null @@ -1,64 +0,0 @@ -// Copyright (c) Umbraco. -// See LICENSE for more details. - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; - -namespace Umbraco.Cms.Core -{ - /// - /// Helper class to deal with Fire and Forget tasks correctly. - /// - public class FireAndForgetTasks - { - private readonly ILogger _logger; - - public FireAndForgetTasks(ILogger logger) => _logger = logger; - - /// - /// Runs a TPL Task fire-and-forget style, the right way - in the - /// background, separate from the current thread, with no risk - /// of it trying to rejoin the current thread. - /// - public Task RunBackgroundTask(Func fn) - { - using (ExecutionContext.SuppressFlow()) // Do not flow AsyncLocal to the child thread - { - Task t = Task.Run(LoggingWrapper(fn)); - t.ConfigureAwait(false); - return t; - } - } - - /// - /// Runs a task fire-and-forget style and notifies the TPL that this - /// will not need a Thread to resume on for a long time, or that there - /// are multiple gaps in thread use that may be long. - /// Use for example when talking to a slow webservice. - /// - public Task RunLongRunningBackgroundTask(Func fn) - { - using (ExecutionContext.SuppressFlow()) // Do not flow AsyncLocal to the child thread - { - Task t = Task.Factory.StartNew(LoggingWrapper(fn), TaskCreationOptions.LongRunning); - t.ConfigureAwait(false); - return t; - } - } - - private Func LoggingWrapper(Func fn) => - async () => - { - try - { - await fn(); - } - catch (Exception e) - { - _logger.LogError(e, "Exception thrown in a background thread"); - } - }; - } -} diff --git a/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs index a8d9f92f4a..9d0bc9ceef 100644 --- a/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs +++ b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs @@ -1,4 +1,4 @@ -using System; +using System; namespace Umbraco.Cms.Core.Scoping { @@ -11,5 +11,6 @@ namespace Umbraco.Cms.Core.Scoping /// Gets the instance unique identifier. /// Guid InstanceId { get; } + int CreatedThreadId { get; } } } diff --git a/src/Umbraco.Core/TaskHelper.cs b/src/Umbraco.Core/TaskHelper.cs new file mode 100644 index 0000000000..ba9f865eba --- /dev/null +++ b/src/Umbraco.Core/TaskHelper.cs @@ -0,0 +1,77 @@ +// Copyright (c) Umbraco. +// See LICENSE for more details. + +using System; +using System.Runtime.CompilerServices; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; + +namespace Umbraco.Cms.Core +{ + /// + /// Helper class to not repeat common patterns with Task. + /// + public sealed class TaskHelper + { + private readonly ILogger _logger; + + public TaskHelper(ILogger logger) => _logger = logger; + + /// + /// Executes a fire and forget task outside of the current execution flow. + /// + public void RunBackgroundTask(Func fn) => ExecuteBackgroundTask(fn); + + // for tests, returning the Task as a public API indicates it can be awaited that is not what we want to do + internal Task ExecuteBackgroundTask(Func fn) + { + // it is also possible to use UnsafeQueueUserWorkItem which does not flow the execution context, + // however that seems more difficult to use for async operations. + + // Do not flow AsyncLocal to the child thread + using (ExecutionContext.SuppressFlow()) + { + // NOTE: ConfigureAwait(false) is irrelevant here, it is not needed because this is not being + // awaited. ConfigureAwait(false) is only relevant when awaiting to prevent the SynchronizationContext + // (very different from the ExecutionContext!) from running the continuation on the calling thread. + return Task.Run(LoggingWrapper(fn)); + } + } + + /// + /// Executes a fire and forget task outside of the current execution flow on a dedicated (non thread-pool) thread. + /// + public void RunLongRunningBackgroundTask(Func fn) => ExecuteLongRunningBackgroundTask(fn); + + // for tests, returning the Task as a public API indicates it can be awaited that is not what we want to do + internal Task ExecuteLongRunningBackgroundTask(Func fn) + { + // it is also possible to use UnsafeQueueUserWorkItem which does not flow the execution context, + // however that seems more difficult to use for async operations. + + // Do not flow AsyncLocal to the child thread + using (ExecutionContext.SuppressFlow()) + { + // NOTE: ConfigureAwait(false) is irrelevant here, it is not needed because this is not being + // awaited. ConfigureAwait(false) is only relevant when awaiting to prevent the SynchronizationContext + // (very different from the ExecutionContext!) from running the continuation on the calling thread. + return Task.Factory.StartNew(LoggingWrapper(fn), TaskCreationOptions.LongRunning); + } + } + + // ensure any exceptions are handled and do not take down the app pool + private Func LoggingWrapper(Func fn) => + async () => + { + try + { + await fn(); + } + catch (Exception e) + { + _logger.LogError(e, "Exception thrown in a background thread"); + } + }; + } +} diff --git a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs index 4f70f28149..f42e88b7df 100644 --- a/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs +++ b/src/Umbraco.Infrastructure/DependencyInjection/UmbracoBuilder.CoreServices.cs @@ -170,7 +170,7 @@ namespace Umbraco.Cms.Infrastructure.DependencyInjection // Services required to run background jobs (with out the handler) builder.Services.AddUnique(); - builder.Services.AddUnique(); + builder.Services.AddUnique(); return builder; } diff --git a/src/Umbraco.Infrastructure/Extensions/InstanceIdentifiableExtensions.cs b/src/Umbraco.Infrastructure/Extensions/InstanceIdentifiableExtensions.cs new file mode 100644 index 0000000000..10f919589a --- /dev/null +++ b/src/Umbraco.Infrastructure/Extensions/InstanceIdentifiableExtensions.cs @@ -0,0 +1,20 @@ +using System; +using System.Collections.Generic; +using System.Text; +using Umbraco.Cms.Core.Scoping; + +namespace Umbraco.Extensions +{ + internal static class InstanceIdentifiableExtensions + { + public static string GetDebugInfo(this IInstanceIdentifiable instance) + { + if (instance == null) + { + return "(NULL)"; + } + + return $"(id: {instance.InstanceId.ToString("N").Substring(0, 8)} from thread: {instance.CreatedThreadId})"; + } + } +} diff --git a/src/Umbraco.Infrastructure/Scoping/Scope.cs b/src/Umbraco.Infrastructure/Scoping/Scope.cs index c68baa5d8c..d239092652 100644 --- a/src/Umbraco.Infrastructure/Scoping/Scope.cs +++ b/src/Umbraco.Infrastructure/Scoping/Scope.cs @@ -1,6 +1,8 @@ using System; using System.Data; +using System.Threading; using Microsoft.Extensions.Logging; +using Umbraco.Extensions; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Events; using Umbraco.Cms.Core.IO; @@ -76,16 +78,29 @@ namespace Umbraco.Cms.Core.Scoping if (detachable) { - if (parent != null) throw new ArgumentException("Cannot set parent on detachable scope.", nameof(parent)); - if (scopeContext != null) throw new ArgumentException("Cannot set context on detachable scope.", nameof(scopeContext)); - if (autoComplete) throw new ArgumentException("Cannot auto-complete a detachable scope.", nameof(autoComplete)); + if (parent != null) + { + throw new ArgumentException("Cannot set parent on detachable scope.", nameof(parent)); + } + + if (scopeContext != null) + { + throw new ArgumentException("Cannot set context on detachable scope.", nameof(scopeContext)); + } + + if (autoComplete) + { + throw new ArgumentException("Cannot auto-complete a detachable scope.", nameof(autoComplete)); + } // detachable creates its own scope context Context = new ScopeContext(); // see note below if (scopeFileSystems == true) + { _fscope = fileSystems.Shadow(); + } return; } @@ -98,16 +113,22 @@ namespace Umbraco.Cms.Core.Scoping // TODO: means that it's OK to go from L2 to None for reading purposes, but writing would be BAD! // this is for XmlStore that wants to bypass caches when rebuilding XML (same for NuCache) if (repositoryCacheMode != RepositoryCacheMode.Unspecified && parent.RepositoryCacheMode > repositoryCacheMode) + { throw new ArgumentException($"Value '{repositoryCacheMode}' cannot be lower than parent value '{parent.RepositoryCacheMode}'.", nameof(repositoryCacheMode)); + } // cannot specify a dispatcher! if (_eventDispatcher != null) + { throw new ArgumentException("Value cannot be specified on nested scope.", nameof(eventDispatcher)); + } // cannot specify a different fs scope! // can be 'true' only on outer scope (and false does not make much sense) if (scopeFileSystems != null && parent._scopeFileSystem != scopeFileSystems) + { throw new ArgumentException($"Value '{scopeFileSystems.Value}' be different from parent value '{parent._scopeFileSystem}'.", nameof(scopeFileSystems)); + } } else { @@ -115,7 +136,9 @@ namespace Umbraco.Cms.Core.Scoping // every scoped FS to trigger the creation of shadow FS "on demand", and that would be // pretty pointless since if scopeFileSystems is true, we *know* we want to shadow if (scopeFileSystems == true) + { _fscope = fileSystems.Shadow(); + } } } @@ -156,6 +179,8 @@ namespace Umbraco.Cms.Core.Scoping public Guid InstanceId { get; } = Guid.NewGuid(); + public int CreatedThreadId { get; } = Thread.CurrentThread.ManagedThreadId; + public ISqlContext SqlContext => _scopeProvider.SqlContext; // a value indicating whether to force call-context @@ -357,11 +382,14 @@ namespace Umbraco.Cms.Core.Scoping { EnsureNotDisposed(); + if (this != _scopeProvider.AmbientScope) { + var failedMessage = $"The {nameof(Scope)} {this.GetDebugInfo()} being disposed is not the Ambient {nameof(Scope)} {_scopeProvider.AmbientScope.GetDebugInfo()}. This typically indicates that a child {nameof(Scope)} was not disposed or flowed to a child thread that was not re-joined to the thread that the parent originated (i.e. Task.Run used as a fire and forget task without ExecutionContext.SuppressFlow())."; + #if DEBUG_SCOPES Scope ambient = _scopeProvider.AmbientScope; - _logger.LogDebug("Dispose error (" + (ambient == null ? "no" : "other") + " ambient)"); + _logger.LogWarning("Dispose error (" + (ambient == null ? "no" : "other") + " ambient)"); if (ambient == null) { throw new InvalidOperationException("Not the ambient scope (no ambient scope)."); @@ -369,11 +397,11 @@ namespace Umbraco.Cms.Core.Scoping ScopeInfo ambientInfos = _scopeProvider.GetScopeInfo(ambient); ScopeInfo disposeInfos = _scopeProvider.GetScopeInfo(this); - throw new InvalidOperationException("Not the ambient scope (see ctor stack traces).\r\n" + throw new InvalidOperationException($"{failedMessage} (see ctor stack traces).\r\n" + "- ambient ->\r\n" + ambientInfos.ToString() + "\r\n" + "- dispose ->\r\n" + disposeInfos.ToString() + "\r\n"); #else - throw new InvalidOperationException("Not the ambient scope."); + throw new InvalidOperationException(failedMessage); #endif } @@ -385,15 +413,20 @@ namespace Umbraco.Cms.Core.Scoping #endif if (_autoComplete && _completed == null) + { _completed = true; + } if (parent != null) + { parent.ChildCompleted(_completed); + } else + { DisposeLastScope(); + } _disposed = true; - GC.SuppressFinalize(this); } private void DisposeLastScope() @@ -489,14 +522,15 @@ namespace Umbraco.Cms.Core.Scoping }); } - private static void TryFinally(params Action[] actions) - { - TryFinally(0, actions); - } + private static void TryFinally(params Action[] actions) => TryFinally(0, actions); private static void TryFinally(int index, Action[] actions) { - if (index == actions.Length) return; + if (index == actions.Length) + { + return; + } + try { actions[index](); diff --git a/src/Umbraco.Infrastructure/Scoping/ScopeContext.cs b/src/Umbraco.Infrastructure/Scoping/ScopeContext.cs index 0ec966d18f..75a9ecb6ad 100644 --- a/src/Umbraco.Infrastructure/Scoping/ScopeContext.cs +++ b/src/Umbraco.Infrastructure/Scoping/ScopeContext.cs @@ -1,6 +1,7 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; +using System.Threading; using Umbraco.Cms.Core.Scoping; namespace Umbraco.Cms.Core.Scoping @@ -41,6 +42,8 @@ namespace Umbraco.Cms.Core.Scoping public Guid InstanceId { get; } = Guid.NewGuid(); + public int CreatedThreadId { get; } = Thread.CurrentThread.ManagedThreadId; + private IDictionary Enlisted => _enlisted ?? (_enlisted = new Dictionary()); diff --git a/src/Umbraco.Infrastructure/Scoping/ScopeProvider.cs b/src/Umbraco.Infrastructure/Scoping/ScopeProvider.cs index 5009f316eb..b026c58ccd 100644 --- a/src/Umbraco.Infrastructure/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Infrastructure/Scoping/ScopeProvider.cs @@ -165,8 +165,14 @@ namespace Umbraco.Cms.Core.Scoping #region Ambient Context - internal const string ContextItemKey = "Umbraco.Core.Scoping.ScopeContext"; + internal static readonly string ContextItemKey = $"{typeof(ScopeProvider).FullName}"; + /// + /// Get or set the Ambient (Current) for the current execution context. + /// + /// + /// The current execution context may be request based (HttpContext) or on a background thread (AsyncLocal) + /// public IScopeContext AmbientContext { get @@ -205,7 +211,12 @@ namespace Umbraco.Cms.Core.Scoping IScope IScopeAccessor.AmbientScope => AmbientScope; - // null if there is none + /// + /// Get or set the Ambient (Current) for the current execution context. + /// + /// + /// The current execution context may be request based (HttpContext) or on a background thread (AsyncLocal) + /// public Scope AmbientScope { // try http context, fallback onto call context @@ -237,6 +248,12 @@ namespace Umbraco.Cms.Core.Scoping #endregion + /// + /// Set the Ambient (Current) and for the current execution context. + /// + /// + /// The current execution context may be request based (HttpContext) or on a background thread (AsyncLocal) + /// public void SetAmbient(Scope scope, IScopeContext context = null) { // clear all @@ -302,12 +319,16 @@ namespace Umbraco.Cms.Core.Scoping /// public IScope DetachScope() { - var ambientScope = AmbientScope; + Scope ambientScope = AmbientScope; if (ambientScope == null) + { throw new InvalidOperationException("There is no ambient scope."); + } if (ambientScope.Detachable == false) + { throw new InvalidOperationException("Ambient scope is not detachable."); + } SetAmbient(ambientScope.OrigScope, ambientScope.OrigContext); ambientScope.OrigScope = null; @@ -508,7 +529,6 @@ namespace Umbraco.Cms.Core.Scoping Scope = scope; Created = DateTime.Now; CtorStack = ctorStack; - CreatedThreadId = System.Threading.Thread.CurrentThread.ManagedThreadId; } public IScope Scope { get; } // the scope itself @@ -516,7 +536,6 @@ namespace Umbraco.Cms.Core.Scoping // the scope's parent identifier public Guid Parent => ((Scope)Scope).ParentScope == null ? Guid.Empty : ((Scope)Scope).ParentScope.InstanceId; - public int CreatedThreadId { get; } // the thread id that created this scope public DateTime Created { get; } // the date time the scope was created public bool Disposed { get; set; } // whether the scope has been disposed already public string Context { get; set; } // the current 'context' that contains the scope (null, "http" or "lcc") @@ -529,10 +548,10 @@ namespace Umbraco.Cms.Core.Scoping .AppendLine("ScopeInfo:") .Append("Instance Id: ") .AppendLine(Scope.InstanceId.ToString()) - .Append("Instance Id: ") + .Append("Parent Id: ") .AppendLine(Parent.ToString()) .Append("Created Thread Id: ") - .AppendLine(CreatedThreadId.ToInvariantString()) + .AppendLine(Scope.CreatedThreadId.ToInvariantString()) .Append("Created At: ") .AppendLine(Created.ToString("O")) .Append("Disposed: ") diff --git a/src/Umbraco.Infrastructure/Search/ExamineComponent.cs b/src/Umbraco.Infrastructure/Search/ExamineComponent.cs index 1e0fbea941..4535bebd8b 100644 --- a/src/Umbraco.Infrastructure/Search/ExamineComponent.cs +++ b/src/Umbraco.Infrastructure/Search/ExamineComponent.cs @@ -28,7 +28,7 @@ namespace Umbraco.Cms.Infrastructure.Search private readonly IValueSetBuilder _mediaValueSetBuilder; private readonly IValueSetBuilder _memberValueSetBuilder; private readonly BackgroundIndexRebuilder _backgroundIndexRebuilder; - private readonly FireAndForgetTasks _taskHelper; + private readonly TaskHelper _taskHelper; private readonly IScopeProvider _scopeProvider; private readonly ServiceContext _services; private readonly IMainDom _mainDom; @@ -53,7 +53,7 @@ namespace Umbraco.Cms.Infrastructure.Search IValueSetBuilder mediaValueSetBuilder, IValueSetBuilder memberValueSetBuilder, BackgroundIndexRebuilder backgroundIndexRebuilder, - FireAndForgetTasks taskHelper) + TaskHelper taskHelper) { _services = services; _scopeProvider = scopeProvider; @@ -657,12 +657,12 @@ namespace Umbraco.Cms.Infrastructure.Search /// private class DeferedReIndexForContent : DeferedAction { - private readonly FireAndForgetTasks _taskHelper; + private readonly TaskHelper _taskHelper; private readonly ExamineComponent _examineComponent; private readonly IContent _content; private readonly bool _isPublished; - public DeferedReIndexForContent(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IContent content, bool isPublished) + public DeferedReIndexForContent(TaskHelper taskHelper, ExamineComponent examineComponent, IContent content, bool isPublished) { _taskHelper = taskHelper; _examineComponent = examineComponent; @@ -672,7 +672,7 @@ namespace Umbraco.Cms.Infrastructure.Search public override void Execute() => Execute(_taskHelper, _examineComponent, _content, _isPublished); - public static void Execute(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IContent content, bool isPublished) + public static void Execute(TaskHelper taskHelper, ExamineComponent examineComponent, IContent content, bool isPublished) => taskHelper.RunBackgroundTask(() => { using IScope scope = examineComponent._scopeProvider.CreateScope(autoComplete: true); @@ -702,12 +702,12 @@ namespace Umbraco.Cms.Infrastructure.Search /// private class DeferedReIndexForMedia : DeferedAction { - private readonly FireAndForgetTasks _taskHelper; + private readonly TaskHelper _taskHelper; private readonly ExamineComponent _examineComponent; private readonly IMedia _media; private readonly bool _isPublished; - public DeferedReIndexForMedia(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IMedia media, bool isPublished) + public DeferedReIndexForMedia(TaskHelper taskHelper, ExamineComponent examineComponent, IMedia media, bool isPublished) { _taskHelper = taskHelper; _examineComponent = examineComponent; @@ -717,7 +717,7 @@ namespace Umbraco.Cms.Infrastructure.Search public override void Execute() => Execute(_taskHelper, _examineComponent, _media, _isPublished); - public static void Execute(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IMedia media, bool isPublished) => + public static void Execute(TaskHelper taskHelper, ExamineComponent examineComponent, IMedia media, bool isPublished) => // perform the ValueSet lookup on a background thread taskHelper.RunBackgroundTask(() => { @@ -744,9 +744,9 @@ namespace Umbraco.Cms.Infrastructure.Search { private readonly ExamineComponent _examineComponent; private readonly IMember _member; - private readonly FireAndForgetTasks _taskHelper; + private readonly TaskHelper _taskHelper; - public DeferedReIndexForMember(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IMember member) + public DeferedReIndexForMember(TaskHelper taskHelper, ExamineComponent examineComponent, IMember member) { _examineComponent = examineComponent; _member = member; @@ -755,7 +755,7 @@ namespace Umbraco.Cms.Infrastructure.Search public override void Execute() => Execute(_taskHelper, _examineComponent, _member); - public static void Execute(FireAndForgetTasks taskHelper, ExamineComponent examineComponent, IMember member) => + public static void Execute(TaskHelper taskHelper, ExamineComponent examineComponent, IMember member) => // perform the ValueSet lookup on a background thread taskHelper.RunBackgroundTask(() => { diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs index 592de6a89c..e42e681a58 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Persistence/LocksTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Data.SqlClient; using System.Linq; @@ -85,10 +85,13 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence }); } - // safe call context ensures that current scope does not leak into starting threads - using (new SafeCallContext()) + // ensure that current scope does not leak into starting threads + using (ExecutionContext.SuppressFlow()) { - foreach (var thread in threads) thread.Start(); + foreach (var thread in threads) + { + thread.Start(); + } } m2.Wait(); @@ -96,13 +99,18 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence var maxAcquired = acquired; m1.Set(); - foreach (var thread in threads) thread.Join(); + foreach (var thread in threads) + { + thread.Join(); + } Assert.AreEqual(threadCount, maxAcquired); Assert.AreEqual(0, acquired); for (var i = 0; i < threadCount; i++) + { Assert.IsNull(exceptions[i]); + } } [Test] @@ -115,7 +123,11 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence var acquired = 0; var entered = 0; var ms = new AutoResetEvent[threadCount]; - for (var i = 0; i < threadCount; i++) ms[i] = new AutoResetEvent(false); + for (var i = 0; i < threadCount; i++) + { + ms[i] = new AutoResetEvent(false); + } + var m1 = new ManualResetEventSlim(false); for (var i = 0; i < threadCount; i++) @@ -153,28 +165,43 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence }); } - // safe call context ensures that current scope does not leak into starting threads - using (new SafeCallContext()) + // ensure that current scope does not leak into starting threads + using (ExecutionContext.SuppressFlow()) { - foreach (var thread in threads) thread.Start(); + foreach (var thread in threads) + { + thread.Start(); + } } m1.Wait(); // all threads have entered ms[0].Set(); // let 0 go Thread.Sleep(100); - for (var i = 1; i < threadCount; i++) ms[i].Set(); // let others go + for (var i = 1; i < threadCount; i++) + { + ms[i].Set(); // let others go + } + Thread.Sleep(500); // only 1 thread has locked Assert.AreEqual(1, acquired); - for (var i = 0; i < threadCount; i++) ms[i].Set(); // let all go + for (var i = 0; i < threadCount; i++) + { + ms[i].Set(); // let all go + } - foreach (var thread in threads) thread.Join(); + foreach (var thread in threads) + { + thread.Join(); + } Assert.AreEqual(0, acquired); for (var i = 0; i < threadCount; i++) + { Assert.IsNull(exceptions[i]); + } } [Retry(10)] // TODO make this test non-flaky. @@ -191,8 +218,8 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence var thread1 = new Thread(() => DeadLockTestThread(1, 2, ev1, ev2, ref e1)); var thread2 = new Thread(() => DeadLockTestThread(2, 1, ev2, ev1, ref e2)); - // safe call context ensures that current scope does not leak into starting threads - using (new SafeCallContext()) + // ensure that current scope does not leak into starting threads + using (ExecutionContext.SuppressFlow()) { thread1.Start(); thread2.Start(); @@ -242,9 +269,13 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence myEv.Set(); if (id1 == 1) + { otherEv.WaitOne(); + } else + { Thread.Sleep(5200); // wait for deadlock... + } Console.WriteLine($"[{id1}] WAIT {id2}"); scope.WriteLock(id2); @@ -275,8 +306,8 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Persistence var thread1 = new Thread(() => NoDeadLockTestThread(1, ev1, ev2, ref e1)); var thread2 = new Thread(() => NoDeadLockTestThread(2, ev2, ev1, ref e1)); - // need safe call context else the current one leaks into *both* threads - using (new SafeCallContext()) + // ensure that current scope does not leak into starting threads + using (ExecutionContext.SuppressFlow()) { thread1.Start(); thread2.Start(); diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeFileSystemsTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeFileSystemsTests.cs index 7529409032..c95417bb2b 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeFileSystemsTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeFileSystemsTests.cs @@ -4,7 +4,10 @@ using System; using System.IO; using System.Text; +using System.Threading; +using System.Threading.Tasks; using Microsoft.Extensions.Logging; +using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Hosting; @@ -27,16 +30,11 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping private IHostingEnvironment HostingEnvironment => GetRequiredService(); [SetUp] - public void SetUp() - { - SafeCallContext.Clear(); - ClearFiles(IOHelper); - } + public void SetUp() => ClearFiles(IOHelper); [TearDown] public void Teardown() { - SafeCallContext.Clear(); FileSystems.ResetShadowId(); ClearFiles(IOHelper); } @@ -115,6 +113,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping string rootUrl = HostingEnvironment.ToAbsolute(GlobalSettings.UmbracoMediaPath); var physMediaFileSystem = new PhysicalFileSystem(IOHelper, HostingEnvironment, GetRequiredService>(), rootPath, rootUrl); IMediaFileSystem mediaFileSystem = MediaFileSystem; + var taskHelper = new TaskHelper(Mock.Of>()); IScopeProvider scopeProvider = ScopeProvider; using (IScope scope = scopeProvider.CreateScope(scopeFileSystems: true)) @@ -127,7 +126,8 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.IsTrue(mediaFileSystem.FileExists("f1.txt")); Assert.IsFalse(physMediaFileSystem.FileExists("f1.txt")); - using (new SafeCallContext()) + // execute on another disconnected thread (execution context will not flow) + Task t = taskHelper.RunBackgroundTask(() => { Assert.IsFalse(mediaFileSystem.FileExists("f1.txt")); @@ -138,7 +138,11 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.IsTrue(mediaFileSystem.FileExists("f2.txt")); Assert.IsTrue(physMediaFileSystem.FileExists("f2.txt")); - } + + return Task.CompletedTask; + }); + + Task.WaitAll(t); Assert.IsTrue(mediaFileSystem.FileExists("f2.txt")); Assert.IsTrue(physMediaFileSystem.FileExists("f2.txt")); @@ -148,10 +152,14 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping [Test] public void SingleShadow() { + var taskHelper = new TaskHelper(Mock.Of>()); IScopeProvider scopeProvider = ScopeProvider; + bool isThrown = false; using (IScope scope = scopeProvider.CreateScope(scopeFileSystems: true)) { - using (new SafeCallContext()) // not nesting! + // This is testing when another thread concurrently tries to create a scoped file system + // because at the moment we don't support concurrent scoped filesystems. + Task t = taskHelper.RunBackgroundTask(() => { // ok to create a 'normal' other scope using (IScope other = scopeProvider.CreateScope()) @@ -160,31 +168,47 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping } // not ok to create a 'scoped filesystems' other scope - // because at the moment we don't support concurrent scoped filesystems + // we will get a "Already shadowing." exception. Assert.Throws(() => - { - IScope other = scopeProvider.CreateScope(scopeFileSystems: true); + { + using IScope other = scopeProvider.CreateScope(scopeFileSystems: true); }); - } + + isThrown = true; + + return Task.CompletedTask; + }); + + Task.WaitAll(t); } + + Assert.IsTrue(isThrown); } [Test] public void SingleShadowEvenDetached() { + var taskHelper = new TaskHelper(Mock.Of>()); var scopeProvider = (ScopeProvider)ScopeProvider; using (IScope scope = scopeProvider.CreateScope(scopeFileSystems: true)) { - using (new SafeCallContext()) // not nesting! + // This is testing when another thread concurrently tries to create a scoped file system + // because at the moment we don't support concurrent scoped filesystems. + Task t = taskHelper.RunBackgroundTask(() => { // not ok to create a 'scoped filesystems' other scope // because at the moment we don't support concurrent scoped filesystems // even a detached one + // we will get a "Already shadowing." exception. Assert.Throws(() => { - IScope other = scopeProvider.CreateDetachedScope(scopeFileSystems: true); + using IScope other = scopeProvider.CreateDetachedScope(scopeFileSystems: true); }); - } + + return Task.CompletedTask; + }); + + Task.WaitAll(t); } IScope detached = scopeProvider.CreateDetachedScope(scopeFileSystems: true); @@ -194,9 +218,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.Throws(() => { // even if there is no ambient scope, there's a single shadow - using (IScope other = scopeProvider.CreateScope(scopeFileSystems: true)) - { - } + using IScope other = scopeProvider.CreateScope(scopeFileSystems: true); }); scopeProvider.AttachScope(detached); diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeTests.cs index 86e717620a..9aaea4b8cc 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Scoping/ScopeTests.cs @@ -2,6 +2,11 @@ // See LICENSE for more details. using System; +using System.Threading; +using System.Threading.Tasks; +using Castle.Core.Logging; +using Microsoft.Extensions.Logging; +using Moq; using NUnit.Framework; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Scoping; @@ -20,6 +25,45 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping [SetUp] public void SetUp() => Assert.IsNull(ScopeProvider.AmbientScope); // gone + [Test] + public void Non_Disposed_Nested_Scope_Throws() + { + ScopeProvider scopeProvider = ScopeProvider; + + Assert.IsNull(ScopeProvider.AmbientScope); + IScope mainScope = scopeProvider.CreateScope(); + + IScope nested = scopeProvider.CreateScope(); // not disposing + + InvalidOperationException ex = Assert.Throws(() => mainScope.Dispose()); + Console.WriteLine(ex); + } + + [Test] + public void Non_Joined_Child_Thread_Nested_Scope_Throws() + { + ScopeProvider scopeProvider = ScopeProvider; + + Assert.IsNull(ScopeProvider.AmbientScope); + IScope mainScope = scopeProvider.CreateScope(); + + // Task.Run will flow the execution context unless ExecutionContext.SuppressFlow() is explicitly called. + // This is what occurs in normal async behavior since it is expected to await (and join) the main thread, + // but if Task.Run is used as a fire and forget thread without being done correctly then the Scope will + // flow to that thread. + var t = Task.Run(() => + { + using IScope nested = scopeProvider.CreateScope(); + Thread.Sleep(2000); // block for a bit + }); + + // now dispose the main without waiting for the child thread to join + InvalidOperationException ex = Assert.Throws(() => mainScope.Dispose()); + + Task.WaitAll(t); + Console.WriteLine(ex); + } + [Test] public void SimpleCreateScope() { @@ -113,9 +157,6 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.IsNotNull(scopeProvider.AmbientScope); Assert.AreSame(scope, scopeProvider.AmbientScope); - // only if Core.DEBUG_SCOPES are defined - //// Assert.IsEmpty(scopeProvider.CallContextObjects); - using (IScope nested = scopeProvider.CreateScope(callContext: true)) { Assert.IsInstanceOf(nested); @@ -126,10 +167,6 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping // it's moved over to call context IScope callContextScope = CallContext.GetData(ScopeProvider.ScopeItemKey); Assert.IsNotNull(callContextScope); - - // only if Core.DEBUG_SCOPES are defined - // var ccnested = scopeProvider.CallContextObjects[callContextKey]; - // Assert.AreSame(nested, ccnested); } // it's naturally back in http context @@ -404,12 +441,15 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping [Test] public void CallContextScope1() { + var taskHelper = new TaskHelper(Mock.Of>()); ScopeProvider scopeProvider = ScopeProvider; using (IScope scope = scopeProvider.CreateScope()) { Assert.IsNotNull(scopeProvider.AmbientScope); Assert.IsNotNull(scopeProvider.AmbientContext); - using (new SafeCallContext()) + + // Run on another thread without a flowed context + Task t = taskHelper.RunBackgroundTask(() => { Assert.IsNull(scopeProvider.AmbientScope); Assert.IsNull(scopeProvider.AmbientContext); @@ -423,7 +463,11 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.IsNull(scopeProvider.AmbientScope); Assert.IsNull(scopeProvider.AmbientContext); - } + + return Task.CompletedTask; + }); + + Task.WaitAll(t); Assert.IsNotNull(scopeProvider.AmbientScope); Assert.AreSame(scope, scopeProvider.AmbientScope); @@ -436,6 +480,7 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping [Test] public void CallContextScope2() { + var taskHelper = new TaskHelper(Mock.Of>()); ScopeProvider scopeProvider = ScopeProvider; Assert.IsNull(scopeProvider.AmbientScope); @@ -443,7 +488,9 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping { Assert.IsNotNull(scopeProvider.AmbientScope); Assert.IsNotNull(scopeProvider.AmbientContext); - using (new SafeCallContext()) + + // Run on another thread without a flowed context + Task t = taskHelper.RunBackgroundTask(() => { Assert.IsNull(scopeProvider.AmbientScope); Assert.IsNull(scopeProvider.AmbientContext); @@ -457,7 +504,10 @@ namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Scoping Assert.IsNull(scopeProvider.AmbientScope); Assert.IsNull(scopeProvider.AmbientContext); - } + return Task.CompletedTask; + }); + + Task.WaitAll(t); Assert.IsNotNull(scopeProvider.AmbientScope); Assert.AreSame(scope, scopeProvider.AmbientScope); diff --git a/src/Umbraco.Tests.UnitTests/Umbraco.Core/FireAndForgetTasksTests.cs b/src/Umbraco.Tests.UnitTests/Umbraco.Core/TaskHelperTests.cs similarity index 77% rename from src/Umbraco.Tests.UnitTests/Umbraco.Core/FireAndForgetTasksTests.cs rename to src/Umbraco.Tests.UnitTests/Umbraco.Core/TaskHelperTests.cs index 2559617a62..a4680387ee 100644 --- a/src/Umbraco.Tests.UnitTests/Umbraco.Core/FireAndForgetTasksTests.cs +++ b/src/Umbraco.Tests.UnitTests/Umbraco.Core/TaskHelperTests.cs @@ -15,13 +15,13 @@ using Umbraco.Cms.Tests.UnitTests.AutoFixture; namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core { [TestFixture] - public class FireAndForgetTasksTests + public class TaskHelperTests { [Test] [AutoMoqData] public void RunBackgroundTask__Suppress_Execution_Context( - [Frozen] ILogger logger, - FireAndForgetTasks sut) + [Frozen] ILogger logger, + TaskHelper sut) { var local = new AsyncLocal { @@ -30,7 +30,7 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core string taskResult = null; - Task t = sut.RunBackgroundTask(() => + Task t = sut.ExecuteBackgroundTask(() => { // FireAndForgetTasks ensure that flow is suppressed therefore this value will be null taskResult = local.Value; @@ -45,11 +45,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core [Test] [AutoMoqData] public void RunBackgroundTask__Must_Run_Func( - [Frozen] ILogger logger, - FireAndForgetTasks sut) + [Frozen] ILogger logger, + TaskHelper sut) { var i = 0; - Task t = sut.RunBackgroundTask(() => + Task t = sut.ExecuteBackgroundTask(() => { Interlocked.Increment(ref i); return Task.CompletedTask; @@ -63,11 +63,11 @@ namespace Umbraco.Cms.Tests.UnitTests.Umbraco.Core [Test] [AutoMoqData] public void RunBackgroundTask__Log_Error_When_Exception_Happen_In_Background_Task( - [Frozen] ILogger logger, + [Frozen] ILogger logger, Exception exception, - FireAndForgetTasks sut) + TaskHelper sut) { - Task t = sut.RunBackgroundTask(() => throw exception); + Task t = sut.ExecuteBackgroundTask(() => throw exception); Task.WaitAll(t);