Fixes and Updates for DB Scope and Ambient Context leaks (#9953)

* Adds some scope tests (ported back from netcore) and provides a much better error message, ensure execution context is not flowed to child tasks that shouldn't leak any current ambient context

* updates comment

* Ensure SqlMainDomLock suppresses execution context too

* Since we're awaiting a task in a library method, ConfigureAwait(false)

* missing null check

Co-authored-by: Elitsa Marinovska <elm@umbraco.dk>

src/Umbraco.Core/Runtime/SqlMainDomLock.cs

@@ -126,7 +126,7 @@ public async Task<bool> AcquireLockAsync(int millisecondsTimeout)
             }
 
 
-            return await WaitForExistingAsync(tempId, millisecondsTimeout);
+            return await WaitForExistingAsync(tempId, millisecondsTimeout).ConfigureAwait(false);
         }
 
         public Task ListenAsync()
@@ -139,13 +139,15 @@ public Task ListenAsync()
 
             // Create a long running task (dedicated thread)
             // to poll to check if we are still the MainDom registered in the DB
-            return Task.Factory.StartNew(
-                ListeningLoop,
-                _cancellationTokenSource.Token,
-                TaskCreationOptions.LongRunning,
-                // Must explicitly specify this, see https://blog.stephencleary.com/2013/10/continuewith-is-dangerous-too.html
-                TaskScheduler.Default);
-
+            using (ExecutionContext.SuppressFlow())
+            {
+                return Task.Factory.StartNew(
+                    ListeningLoop,
+                    _cancellationTokenSource.Token,
+                    TaskCreationOptions.LongRunning,
+                    // Must explicitly specify this, see https://blog.stephencleary.com/2013/10/continuewith-is-dangerous-too.html
+                    TaskScheduler.Default);
+            }
         }
 
         /// <summary>
@@ -226,10 +228,10 @@ private void ListeningLoop()
                         }
                     }
                     finally
-                    {
-                        db?.CompleteTransaction();
-                        db?.Dispose();
-                    }
+                        {
+                            db?.CompleteTransaction();
+                            db?.Dispose();
+                        }
                 }
 
             }
@@ -245,37 +247,40 @@ private Task<bool> WaitForExistingAsync(string tempId, int millisecondsTimeout)
         {
             var updatedTempId = tempId + UpdatedSuffix;
 
-            return Task.Run(() =>
+            using (ExecutionContext.SuppressFlow())
             {
-                try
+                return Task.Run(() =>
                 {
-                    using var db = _dbFactory.CreateDatabase();
-
-                    var watch = new Stopwatch();
-                    watch.Start();
-                    while (true)
+                    try
                     {
-                        // poll very often, we need to take over as fast as we can
-                        // local testing shows the actual query to be executed from client/server is approx 300ms but would change depending on environment/IO
-                        Thread.Sleep(1000);
-
-                        var acquired = TryAcquire(db, tempId, updatedTempId);
-                        if (acquired.HasValue)
-                            return acquired.Value;
+                        using var db = _dbFactory.CreateDatabase();
 
-                        if (watch.ElapsedMilliseconds >= millisecondsTimeout)
+                        var watch = new Stopwatch();
+                        watch.Start();
+                        while (true)
                         {
-                            return AcquireWhenMaxWaitTimeElapsed(db);
+                            // poll very often, we need to take over as fast as we can
+                            // local testing shows the actual query to be executed from client/server is approx 300ms but would change depending on environment/IO
+                            Thread.Sleep(1000);
+
+                            var acquired = TryAcquire(db, tempId, updatedTempId);
+                            if (acquired.HasValue)
+                                return acquired.Value;
+
+                            if (watch.ElapsedMilliseconds >= millisecondsTimeout)
+                            {
+                                return AcquireWhenMaxWaitTimeElapsed(db);
+                            }
                         }
                     }
-                }
-                catch (Exception ex)
-                {
-                    _logger.Error<SqlMainDomLock>(ex, "An error occurred trying to acquire and waiting for existing SqlMainDomLock to shutdown");
-                    return false;
-                }
+                    catch (Exception ex)
+                    {
+                        _logger.Error<SqlMainDomLock>(ex, "An error occurred trying to acquire and waiting for existing SqlMainDomLock to shutdown");
+                        return false;
+                    }
 
-            }, _cancellationTokenSource.Token);
+                }, _cancellationTokenSource.Token);
+            }
         }
 
         private bool? TryAcquire(IUmbracoDatabase db, string tempId, string updatedTempId)

src/Umbraco.Core/Scoping/Scope.cs

@@ -345,6 +345,8 @@ public void Dispose()
 
             if (this != _scopeProvider.AmbientScope)
             {
+                var failedMessage = $"The {nameof(Scope)} {this.InstanceId} being disposed is not the Ambient {nameof(Scope)} {(_scopeProvider.AmbientScope?.InstanceId.ToString() ?? "NULL")}. This typically indicates that a child {nameof(Scope)} was not disposed, or flowed to a child thread that was not awaited, or concurrent threads are accessing the same {nameof(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().";
+
 #if DEBUG_SCOPES
                 var ambient = _scopeProvider.AmbientScope;
                 _logger.Debug<Scope>("Dispose error (" + (ambient == null ? "no" : "other") + " ambient)");
@@ -356,7 +358,7 @@ public void Dispose()
                     + "- ambient ctor ->\r\n" + ambientInfos.CtorStack + "\r\n"
                     + "- dispose ctor ->\r\n" + disposeInfos.CtorStack + "\r\n");
 #else
-                throw new InvalidOperationException("Not the ambient scope.");
+                throw new InvalidOperationException(failedMessage);
 #endif
             }
 

src/Umbraco.Core/Scoping/ScopeProvider.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.ComponentModel;
 using System.Data;
 using System.Runtime.Remoting.Messaging;
 using System.Web;
@@ -240,6 +241,9 @@ public ScopeContext AmbientContext
                 var value = GetHttpContextObject<ScopeContext>(ContextItemKey, false);
                 return value ?? GetCallContextObject<ScopeContext>(ContextItemKey);
             }
+
+            [Obsolete("This setter is not used and will be removed in future versions")]
+            [EditorBrowsable(EditorBrowsableState.Never)]
             set
             {
                 // clear both

src/Umbraco.Examine/UmbracoExamineIndex.cs

@@ -24,8 +24,8 @@ public abstract class UmbracoExamineIndex : LuceneIndex, IUmbracoIndex, IIndexDi
         // note
         // wrapping all operations that end up calling base.SafelyProcessQueueItems in a safe call
         // context because they will fork a thread/task/whatever which should *not* capture our
-        // call context (and the database it can contain)! ideally we should be able to override
-        // SafelyProcessQueueItems but that's not possible in the current version of Examine.
+        // call context (and the database it can contain)!
+        // TODO: FIX Examine to not flow the ExecutionContext so callers don't need to worry about this!
 
         /// <summary>
         /// Used to store the path of a content object
@@ -99,13 +99,30 @@ protected override void PerformDeleteFromIndex(IEnumerable<string> itemIds, Acti
         {
             if (CanInitialize())
             {
+                // Use SafeCallContext to prevent the current CallContext flow to child
+                // tasks executed in the base class so we don't leak Scopes.
+                // TODO: See notes at the top of this class
                 using (new SafeCallContext())
                 {
                     base.PerformDeleteFromIndex(itemIds, onComplete);
                 }
             }
         }
 
+        protected override void PerformIndexItems(IEnumerable<ValueSet> values, Action<IndexOperationEventArgs> onComplete)
+        {
+            if (CanInitialize())
+            {
+                // Use SafeCallContext to prevent the current CallContext flow to child
+                // tasks executed in the base class so we don't leak Scopes.
+                // TODO: See notes at the top of this class
+                using (new SafeCallContext())
+                {
+                    base.PerformIndexItems(values, onComplete);
+                }
+            }
+        }
+
         /// <summary>
         /// Returns true if the Umbraco application is in a state that we can initialize the examine indexes
         /// </summary>

src/Umbraco.Tests/Scoping/ScopeTests.cs

@@ -2,6 +2,7 @@
 using System.Collections;
 using System.Runtime.Remoting.Messaging;
 using System.Threading;
+using System.Threading.Tasks;
 using NUnit.Framework;
 using Umbraco.Core;
 using Umbraco.Core.Persistence;
@@ -24,6 +25,119 @@ public override void SetUp()
             Assert.IsNull(ScopeProvider.AmbientScope); // gone
         }
 
+        [Test]
+        public void GivenUncompletedScopeOnChildThread_WhenTheParentCompletes_TheTransactionIsRolledBack()
+        {
+            ScopeProvider scopeProvider = ScopeProvider;
+
+            Assert.IsNull(ScopeProvider.AmbientScope);
+            IScope mainScope = scopeProvider.CreateScope();
+
+            var t = Task.Run(() =>
+            {
+                IScope nested = scopeProvider.CreateScope();
+                Thread.Sleep(2000);
+                nested.Dispose();
+            });
+
+            Thread.Sleep(1000); // mimic some long running operation that is shorter than the other thread
+            mainScope.Complete();
+            Assert.Throws<InvalidOperationException>(() => mainScope.Dispose());
+
+            Task.WaitAll(t);
+        }
+
+        [Test]
+        public void GivenNonDisposedChildScope_WhenTheParentDisposes_ThenInvalidOperationExceptionThrows()
+        {
+            // this all runs in the same execution context so the AmbientScope reference isn't a copy
+
+            ScopeProvider scopeProvider = ScopeProvider;
+
+            Assert.IsNull(ScopeProvider.AmbientScope);
+            IScope mainScope = scopeProvider.CreateScope();
+
+            IScope nested = scopeProvider.CreateScope(); // not disposing
+
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => mainScope.Dispose());
+            Console.WriteLine(ex);
+        }
+
+        [Test]
+        public void GivenChildThread_WhenParentDisposedBeforeChild_ParentScopeThrows()
+        {           
+            // The ambient context is NOT thread safe, even though it has locks, etc...
+            // This all just goes to show that concurrent threads with scopes is a no-go.
+            var childWait = new ManualResetEventSlim(false);
+            var parentWait = new ManualResetEventSlim(false);
+
+            ScopeProvider scopeProvider = ScopeProvider;
+
+            Assert.IsNull(ScopeProvider.AmbientScope);
+            IScope mainScope = scopeProvider.CreateScope();
+
+            var t = Task.Run(() =>
+            {
+                Console.WriteLine("Child Task start: " + scopeProvider.AmbientScope.InstanceId);                
+                // This will evict the parent from the ScopeProvider.StaticCallContextObjects
+                // and replace it with the child
+                IScope nested = scopeProvider.CreateScope();
+                childWait.Set();
+                Console.WriteLine("Child Task scope created: " + scopeProvider.AmbientScope.InstanceId);
+                parentWait.Wait(); // wait for the parent thread
+                Console.WriteLine("Child Task before dispose: " + scopeProvider.AmbientScope.InstanceId);
+                // This will evict the child from the ScopeProvider.StaticCallContextObjects
+                // and replace it with the parent
+                nested.Dispose();
+                Console.WriteLine("Child Task after dispose: " + scopeProvider.AmbientScope.InstanceId);
+            });
+
+            childWait.Wait(); // wait for the child to start and create the scope
+            // This is a confusing thing (this is not the case in netcore), this is NULL because the
+            // parent thread's scope ID was evicted from the ScopeProvider.StaticCallContextObjects
+            // so now the ambient context is null because the GUID in the CallContext doesn't match
+            // the GUID in the ScopeProvider.StaticCallContextObjects.
+            Assert.IsNull(scopeProvider.AmbientScope);
+            // now dispose the main without waiting for the child thread to join
+            // This will throw because at this stage a child scope has been created which means
+            // it is the Ambient (top) scope but here we're trying to dispose the non top scope.
+            Assert.Throws<InvalidOperationException>(() => mainScope.Dispose());
+            parentWait.Set();      // tell child thread to proceed
+            Task.WaitAll(t);        // wait for the child to dispose
+            mainScope.Dispose();    // now it's ok
+            Console.WriteLine("Parent Task disposed: " + scopeProvider.AmbientScope?.InstanceId);
+        }
+
+        [Test]
+        public void GivenChildThread_WhenChildDisposedBeforeParent_OK()
+        {
+            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(() =>
+            {
+                Console.WriteLine("Child Task start: " + scopeProvider.AmbientScope.InstanceId);
+                IScope nested = scopeProvider.CreateScope();                
+                Console.WriteLine("Child Task before dispose: " + scopeProvider.AmbientScope.InstanceId);
+                nested.Dispose();
+                Console.WriteLine("Child Task after disposed: " + scopeProvider.AmbientScope.InstanceId);
+            });
+
+            Console.WriteLine("Parent Task waiting: " + scopeProvider.AmbientScope?.InstanceId);
+            Task.WaitAll(t);
+            Console.WriteLine("Parent Task disposing: " + scopeProvider.AmbientScope.InstanceId);
+            mainScope.Dispose();
+            Console.WriteLine("Parent Task disposed: " + scopeProvider.AmbientScope?.InstanceId);
+
+            Assert.Pass();
+        }
+
         [Test]
         public void SimpleCreateScope()
         {