From 10a2303458519fff6162d667cd14927f3a9ef79a Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 18 Oct 2019 10:19:11 +1100 Subject: [PATCH 01/22] Renames AsyncLock -> SystemLock --- src/Umbraco.Core/MainDom.cs | 4 +-- .../{AsyncLock.cs => SystemLock.cs} | 35 ++----------------- .../SafeXmlReaderWriter.cs | 2 +- .../LegacyXmlPublishedCache/XmlStore.cs | 2 +- .../XmlStoreFilePersister.cs | 2 +- 5 files changed, 8 insertions(+), 37 deletions(-) rename src/Umbraco.Core/{AsyncLock.cs => SystemLock.cs} (80%) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index d1012fb669..613abd6946 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -25,7 +25,7 @@ namespace Umbraco.Core private readonly object _locko = new object(); // async lock representing the main domain lock - private readonly AsyncLock _asyncLock; + private readonly SystemLock _asyncLock; private IDisposable _asyncLocker; // event wait handle used to notify current main domain that it should @@ -68,7 +68,7 @@ namespace Umbraco.Core var hash = (appId + ":::" + appPath).ToSHA1(); var lockName = "UMBRACO-" + hash + "-MAINDOM-LCK"; - _asyncLock = new AsyncLock(lockName); + _asyncLock = new SystemLock(lockName); var eventName = "UMBRACO-" + hash + "-MAINDOM-EVT"; _signal = new EventWaitHandle(false, EventResetMode.AutoReset, eventName); diff --git a/src/Umbraco.Core/AsyncLock.cs b/src/Umbraco.Core/SystemLock.cs similarity index 80% rename from src/Umbraco.Core/AsyncLock.cs rename to src/Umbraco.Core/SystemLock.cs index 6dd866705e..e637c7b08f 100644 --- a/src/Umbraco.Core/AsyncLock.cs +++ b/src/Umbraco.Core/SystemLock.cs @@ -21,18 +21,18 @@ namespace Umbraco.Core // been closed, the Semaphore system object is destroyed - so in any case // an iisreset should clean up everything // - internal class AsyncLock + internal class SystemLock { private readonly SemaphoreSlim _semaphore; private readonly Semaphore _semaphore2; private readonly IDisposable _releaser; private readonly Task _releaserTask; - public AsyncLock() + public SystemLock() : this (null) { } - public AsyncLock(string name) + public SystemLock(string name) { // WaitOne() waits until count > 0 then decrements count // Release() increments count @@ -67,35 +67,6 @@ namespace Umbraco.Core : new NamedSemaphoreReleaser(_semaphore2); } - //NOTE: We don't use the "Async" part of this lock at all - //TODO: Remove this and rename this class something like SystemWideLock, then we can re-instate this logic if we ever need an Async lock again - - //public Task LockAsync() - //{ - // var wait = _semaphore != null - // ? _semaphore.WaitAsync() - // : _semaphore2.WaitOneAsync(); - - // return wait.IsCompleted - // ? _releaserTask ?? Task.FromResult(CreateReleaser()) // anonymous vs named - // : wait.ContinueWith((_, state) => (((AsyncLock) state).CreateReleaser()), - // this, CancellationToken.None, - // TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); - //} - - //public Task LockAsync(int millisecondsTimeout) - //{ - // var wait = _semaphore != null - // ? _semaphore.WaitAsync(millisecondsTimeout) - // : _semaphore2.WaitOneAsync(millisecondsTimeout); - - // return wait.IsCompleted - // ? _releaserTask ?? Task.FromResult(CreateReleaser()) // anonymous vs named - // : wait.ContinueWith((_, state) => (((AsyncLock)state).CreateReleaser()), - // this, CancellationToken.None, - // TaskContinuationOptions.ExecuteSynchronously, TaskScheduler.Default); - //} - public IDisposable Lock() { if (_semaphore != null) diff --git a/src/Umbraco.Tests/LegacyXmlPublishedCache/SafeXmlReaderWriter.cs b/src/Umbraco.Tests/LegacyXmlPublishedCache/SafeXmlReaderWriter.cs index c0b9383b57..aa88f28dc0 100644 --- a/src/Umbraco.Tests/LegacyXmlPublishedCache/SafeXmlReaderWriter.cs +++ b/src/Umbraco.Tests/LegacyXmlPublishedCache/SafeXmlReaderWriter.cs @@ -39,7 +39,7 @@ namespace Umbraco.Tests.LegacyXmlPublishedCache return scopeProvider?.Context?.GetEnlisted(EnlistKey); } - public static SafeXmlReaderWriter Get(IScopeProvider scopeProvider, AsyncLock xmlLock, XmlDocument xml, Action refresh, Action apply, bool writer) + public static SafeXmlReaderWriter Get(IScopeProvider scopeProvider, SystemLock xmlLock, XmlDocument xml, Action refresh, Action apply, bool writer) { var scopeContext = scopeProvider.Context; diff --git a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStore.cs b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStore.cs index 3b675c2f07..01f1ce264c 100644 --- a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStore.cs +++ b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStore.cs @@ -305,7 +305,7 @@ namespace Umbraco.Tests.LegacyXmlPublishedCache private XmlDocument _xmlDocument; // supplied xml document (for tests) private volatile XmlDocument _xml; // master xml document - private readonly AsyncLock _xmlLock = new AsyncLock(); // protects _xml + private readonly SystemLock _xmlLock = new SystemLock(); // protects _xml // to be used by PublishedContentCache only // for non-preview content only diff --git a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStoreFilePersister.cs b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStoreFilePersister.cs index 145a19872a..56c09b18ac 100644 --- a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStoreFilePersister.cs +++ b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlStoreFilePersister.cs @@ -24,7 +24,7 @@ namespace Umbraco.Tests.LegacyXmlPublishedCache private bool _released; private Timer _timer; private DateTime _initialTouch; - private readonly AsyncLock _runLock = new AsyncLock(); // ensure we run once at a time + private readonly SystemLock _runLock = new SystemLock(); // ensure we run once at a time // note: // as long as the runner controls the runs, we know that we run once at a time, but From 81ae83556493e8f4dc66fc08a40a3e62295b6582 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 18 Oct 2019 10:30:58 +1100 Subject: [PATCH 02/22] Implements Disposable pattern correctly for the NamedSemaphoreReleaser --- src/Umbraco.Core/SystemLock.cs | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/SystemLock.cs b/src/Umbraco.Core/SystemLock.cs index e637c7b08f..a04eb2f980 100644 --- a/src/Umbraco.Core/SystemLock.cs +++ b/src/Umbraco.Core/SystemLock.cs @@ -100,6 +100,12 @@ namespace Umbraco.Core _handle = GCHandle.Alloc(_semaphore); } + #region IDisposable Support + + // This code added to correctly implement the disposable pattern. + + private bool disposedValue = false; // To detect redundant calls + public void Dispose() { Dispose(true); @@ -108,12 +114,21 @@ namespace Umbraco.Core private void Dispose(bool disposing) { - // critical - _handle.Free(); - _semaphore.Release(); - _semaphore.Dispose(); - } + if (!disposedValue) + { + if (disposing) + { + _semaphore.Release(); + _semaphore.Dispose(); + } + // free unmanaged resources (unmanaged objects) and override a finalizer below. + _handle.Free(); + + disposedValue = true; + } + } + // we WANT to release the semaphore because it's a system object, ie a critical // non-managed resource - and if it is not released then noone else can acquire // the lock - so we inherit from CriticalFinalizerObject which means that the @@ -142,6 +157,9 @@ namespace Umbraco.Core // we do NOT want the finalizer to throw - never ever } } + + #endregion + } private class SemaphoreSlimReleaser : IDisposable From b5f29f2390fa346a8edc098d5e42b0bef4ce89fe Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 18 Oct 2019 11:08:58 +1100 Subject: [PATCH 03/22] Ensure the EventWaitHandle is cleaned up, MainDom and BackgroundTaskRunner was not using HostingEnvironment.Stop correctly, ensures no exception is thrown when setting task result on a background task, ensure multiple terminations don't occur, ensure terminations are done if shutdown fails. --- src/Umbraco.Core/MainDom.cs | 41 ++++++++++++++++--- src/Umbraco.Core/SystemLock.cs | 2 + src/Umbraco.Core/Umbraco.Core.csproj | 2 +- .../Scheduling/BackgroundTaskRunner.cs | 41 ++++++++++++++----- 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 613abd6946..41ed8eb242 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -15,7 +15,7 @@ namespace Umbraco.Core /// When an AppDomain starts, it tries to acquire the main domain status. /// When an AppDomain stops (eg the application is restarting) it should release the main domain status. /// - internal class MainDom : IMainDom, IRegisteredObject + internal class MainDom : IMainDom, IRegisteredObject, IDisposable { #region Vars @@ -204,14 +204,43 @@ namespace Umbraco.Core // IRegisteredObject void IRegisteredObject.Stop(bool immediate) { - try - { - OnSignal("environment"); // will run once - } - finally + OnSignal("environment"); // will run once + + if (immediate) { + //only unregister when it's the final call, else we won't be notified of the final call HostingEnvironment.UnregisterObject(this); + + // The web app is stopping immediately, dispose eagerly + Dispose(true); } } + + #region IDisposable Support + + // This code added to correctly implement the disposable pattern. + + private bool disposedValue = false; // To detect redundant calls + + protected virtual void Dispose(bool disposing) + { + if (!disposedValue) + { + if (disposing) + { + _signal?.Close(); + _signal?.Dispose(); + } + + disposedValue = true; + } + } + + public void Dispose() + { + Dispose(true); + } + + #endregion } } diff --git a/src/Umbraco.Core/SystemLock.cs b/src/Umbraco.Core/SystemLock.cs index a04eb2f980..04150d58c1 100644 --- a/src/Umbraco.Core/SystemLock.cs +++ b/src/Umbraco.Core/SystemLock.cs @@ -120,6 +120,8 @@ namespace Umbraco.Core { _semaphore.Release(); _semaphore.Dispose(); + + } // free unmanaged resources (unmanaged objects) and override a finalizer below. diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index c19722433f..2b9631c747 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -122,7 +122,7 @@ --> - + diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index 47e97593f0..dab6a8865e 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -700,16 +700,23 @@ namespace Umbraco.Web.Scheduling // processing asynchronously before calling the UnregisterObject method. _logger.Info("{LogPrefix} Waiting for tasks to complete", _logPrefix); - Shutdown(false, false); // do not accept any more tasks, flush the queue, do not wait - // raise the completed event only after the running threading task has completed - lock (_locker) + try { - if (_runningTask != null) - _runningTask.ContinueWith(_ => Terminate(false)); - else - Terminate(false); + Shutdown(false, false); // do not accept any more tasks, flush the queue, do not wait } + finally + { + // raise the completed event only after the running threading task has completed + lock (_locker) + { + if (_runningTask != null) + _runningTask.ContinueWith(_ => Terminate(false)); + else + Terminate(false); + } + } + } else { @@ -719,8 +726,14 @@ namespace Umbraco.Web.Scheduling // otherwise, its registration will be removed by the application manager. _logger.Info("{LogPrefix} Canceling tasks", _logPrefix); - Shutdown(true, true); // cancel all tasks, wait for the current one to end - Terminate(true); + try + { + Shutdown(true, true); // cancel all tasks, wait for the current one to end + } + finally + { + Terminate(true); + } } } @@ -732,7 +745,13 @@ namespace Umbraco.Web.Scheduling // raise the Terminated event // complete the awaitable completion source, if any - HostingEnvironment.UnregisterObject(this); + if (immediate) + { + //only unregister when it's the final call, else we won't be notified of the final call + HostingEnvironment.UnregisterObject(this); + } + + if (_terminated) return; // already taken care of TaskCompletionSource terminatedSource; lock (_locker) @@ -747,7 +766,7 @@ namespace Umbraco.Web.Scheduling OnEvent(Terminated, "Terminated"); - terminatedSource.SetResult(0); + terminatedSource.TrySetResult(0); } } } From 44ed611b7a3f5bc2542df9823a572cb8a9914391 Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 18 Oct 2019 11:45:09 +1100 Subject: [PATCH 04/22] Ensure MainDom is always registered with HostingEnvironment, not just when it acquires maindom --- src/Umbraco.Core/MainDom.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 41ed8eb242..10096567f3 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -25,8 +25,8 @@ namespace Umbraco.Core private readonly object _locko = new object(); // async lock representing the main domain lock - private readonly SystemLock _asyncLock; - private IDisposable _asyncLocker; + private readonly SystemLock _systemLock; + private IDisposable _systemLocker; // event wait handle used to notify current main domain that it should // release the lock because a new domain wants to be the main domain @@ -48,6 +48,8 @@ namespace Umbraco.Core // initializes a new instance of MainDom public MainDom(ILogger logger) { + HostingEnvironment.RegisterObject(this); + _logger = logger; var appId = string.Empty; @@ -68,7 +70,7 @@ namespace Umbraco.Core var hash = (appId + ":::" + appPath).ToSHA1(); var lockName = "UMBRACO-" + hash + "-MAINDOM-LCK"; - _asyncLock = new SystemLock(lockName); + _systemLock = new SystemLock(lockName); var eventName = "UMBRACO-" + hash + "-MAINDOM-EVT"; _signal = new EventWaitHandle(false, EventResetMode.AutoReset, eventName); @@ -142,7 +144,7 @@ namespace Umbraco.Core { // in any case... _isMainDom = false; - _asyncLocker.Dispose(); + _systemLocker?.Dispose(); _logger.Info("Released ({SignalSource})", source); } } @@ -173,7 +175,7 @@ namespace Umbraco.Core // and the other one will timeout, which is accepted //TODO: This can throw a TimeoutException - in which case should this be in a try/finally to ensure the signal is always reset? - _asyncLocker = _asyncLock.Lock(LockTimeoutMilliseconds); + _systemLocker = _systemLock.Lock(LockTimeoutMilliseconds); _isMainDom = true; // we need to reset the event, because otherwise we would end up @@ -189,8 +191,6 @@ namespace Umbraco.Core _signal.WaitOneAsync() .ContinueWith(_ => OnSignal("signal")); - HostingEnvironment.RegisterObject(this); - _logger.Info("Acquired."); return true; } From beb16a2f79b847dcc2cb6de5752d7e56b2ede3ce Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 21 Oct 2019 15:47:37 +1100 Subject: [PATCH 05/22] Ensures MainDom.Register absolutely does not call install logic if it's not MainDom, adds logging --- src/Umbraco.Core/MainDom.cs | 6 ++++++ .../PublishedCache/NuCache/PublishedSnapshotService.cs | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 10096567f3..e4beafc110 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -101,6 +101,12 @@ namespace Umbraco.Core lock (_locko) { if (_signaled) return false; + if (_isMainDom == false) + { + _logger.Warn("Register called when MainDom has not been acquired"); + return false; + } + install?.Invoke(); if (release != null) _callbacks.Add(new KeyValuePair(weight, release)); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 6e7916c77f..3fcf61a7bc 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -132,6 +132,8 @@ namespace Umbraco.Web.PublishedCache.NuCache // if both local databases exist then GetTree will open them, else new databases will be created _localContentDb = BTree.GetTree(localContentDbPath, _localDbExists); _localMediaDb = BTree.GetTree(localMediaDbPath, _localDbExists); + + _logger.Info($"Registered with MainDom, local db exists? {_localDbExists}"); }, () => { @@ -144,6 +146,8 @@ namespace Umbraco.Web.PublishedCache.NuCache _mediaStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned _localMediaDb = null; } + + _logger.Info("Released from MainDom"); }); // stores are created with a db so they can write to it, but they do not read from it, From eb670fd1ac91c8d1074a2897cf0c253783dfe0e8 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 21 Oct 2019 15:59:25 +1100 Subject: [PATCH 06/22] ensure OnSignal logic is all run within the lock, ensures all actions are run even if one throw on shutdown. --- src/Umbraco.Core/MainDom.cs | 46 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index e4beafc110..f2505c3f78 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -126,32 +126,32 @@ namespace Umbraco.Core if (_signaled) return; if (_isMainDom == false) return; // probably not needed _signaled = true; - } - try - { - _logger.Info("Stopping ({SignalSource})", source); - foreach (var callback in _callbacks.OrderBy(x => x.Key).Select(x => x.Value)) + try { - try + _logger.Info("Stopping ({SignalSource})", source); + foreach (var callback in _callbacks.OrderBy(x => x.Key).Select(x => x.Value)) { - callback(); // no timeout on callbacks + try + { + callback(); // no timeout on callbacks + } + catch (Exception e) + { + _logger.Error(e, "Error while running callback"); + continue; + } } - catch (Exception e) - { - _logger.Error(e, "Error while running callback, remaining callbacks will not run."); - throw; - } - + _logger.Debug("Stopped ({SignalSource})", source); } - _logger.Debug("Stopped ({SignalSource})", source); - } - finally - { - // in any case... - _isMainDom = false; - _systemLocker?.Dispose(); - _logger.Info("Released ({SignalSource})", source); + finally + { + // in any case... + _isMainDom = false; + _systemLocker?.Dispose(); + _logger.Info("Released ({SignalSource})", source); + } + } } @@ -211,7 +211,7 @@ namespace Umbraco.Core void IRegisteredObject.Stop(bool immediate) { OnSignal("environment"); // will run once - + if (immediate) { //only unregister when it's the final call, else we won't be notified of the final call @@ -241,7 +241,7 @@ namespace Umbraco.Core disposedValue = true; } } - + public void Dispose() { Dispose(true); From 7afdf9e9a94ffb81828c35ad75345ee86132ab4d Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 22 Oct 2019 11:40:51 +1100 Subject: [PATCH 07/22] ensure maindom is disposed as soon as the hosting environment is signaled, remove the GCHandle --- src/Umbraco.Core/MainDom.cs | 6 +++--- src/Umbraco.Core/SystemLock.cs | 23 +++++++++++------------ 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index f2505c3f78..ccecb4aa82 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -212,13 +212,13 @@ namespace Umbraco.Core { OnSignal("environment"); // will run once + // The web app is stopping, dispose eagerly + Dispose(true); + if (immediate) { //only unregister when it's the final call, else we won't be notified of the final call HostingEnvironment.UnregisterObject(this); - - // The web app is stopping immediately, dispose eagerly - Dispose(true); } } diff --git a/src/Umbraco.Core/SystemLock.cs b/src/Umbraco.Core/SystemLock.cs index 04150d58c1..4eaae7082b 100644 --- a/src/Umbraco.Core/SystemLock.cs +++ b/src/Umbraco.Core/SystemLock.cs @@ -29,7 +29,7 @@ namespace Umbraco.Core private readonly Task _releaserTask; public SystemLock() - : this (null) + : this(null) { } public SystemLock(string name) @@ -92,12 +92,10 @@ namespace Umbraco.Core private class NamedSemaphoreReleaser : CriticalFinalizerObject, IDisposable { private readonly Semaphore _semaphore; - private GCHandle _handle; internal NamedSemaphoreReleaser(Semaphore semaphore) { _semaphore = semaphore; - _handle = GCHandle.Alloc(_semaphore); } #region IDisposable Support @@ -116,21 +114,22 @@ namespace Umbraco.Core { if (!disposedValue) { - if (disposing) + try { _semaphore.Release(); - _semaphore.Dispose(); - - } - - // free unmanaged resources (unmanaged objects) and override a finalizer below. - _handle.Free(); - + finally + { + try + { + _semaphore.Dispose(); + } + catch { } + } disposedValue = true; } } - + // we WANT to release the semaphore because it's a system object, ie a critical // non-managed resource - and if it is not released then noone else can acquire // the lock - so we inherit from CriticalFinalizerObject which means that the From 7ace5baf9b2439ced8d86251e65a5a5be8b308da Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Thu, 17 Oct 2019 18:46:11 +0100 Subject: [PATCH 08/22] Allow nucache content and/or media db files to be reused if they already exist. --- src/Umbraco.Core/Runtime/CoreRuntime.cs | 28 ++++++++++--------- .../NuCache/PublishedSnapshotService.cs | 8 ++++-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Core/Runtime/CoreRuntime.cs b/src/Umbraco.Core/Runtime/CoreRuntime.cs index 5b069641c4..5cca74124d 100644 --- a/src/Umbraco.Core/Runtime/CoreRuntime.cs +++ b/src/Umbraco.Core/Runtime/CoreRuntime.cs @@ -140,23 +140,25 @@ namespace Umbraco.Core.Runtime Compose(composition); // acquire the main domain - if this fails then anything that should be registered with MainDom will not operate - AcquireMainDom(mainDom); + if (AcquireMainDom(mainDom)) + { + // determine our runtime level + DetermineRuntimeLevel(databaseFactory, ProfilingLogger); - // determine our runtime level - DetermineRuntimeLevel(databaseFactory, ProfilingLogger); + // get composers, and compose + var composerTypes = ResolveComposerTypes(typeLoader); + composition.WithCollectionBuilder(); + var composers = new Composers(composition, composerTypes, ProfilingLogger); + composers.Compose(); - // get composers, and compose - var composerTypes = ResolveComposerTypes(typeLoader); - composition.WithCollectionBuilder(); - var composers = new Composers(composition, composerTypes, ProfilingLogger); - composers.Compose(); + // create the factory + _factory = Current.Factory = composition.CreateFactory(); - // create the factory - _factory = Current.Factory = composition.CreateFactory(); + // create & initialize the components + _components = _factory.GetInstance(); + _components.Initialize(); + } - // create & initialize the components - _components = _factory.GetInstance(); - _components.Initialize(); } catch (Exception e) { diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 3fcf61a7bc..81a3aa9e21 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -128,10 +128,12 @@ namespace Umbraco.Web.PublishedCache.NuCache var path = GetLocalFilesPath(); var localContentDbPath = Path.Combine(path, "NuCache.Content.db"); var localMediaDbPath = Path.Combine(path, "NuCache.Media.db"); - _localDbExists = File.Exists(localContentDbPath) && File.Exists(localMediaDbPath); + var localContentDbExists = File.Exists(localContentDbPath); + var localMediaDbExists = File.Exists(localMediaDbPath); + _localDbExists = localContentDbExists && localMediaDbExists; // if both local databases exist then GetTree will open them, else new databases will be created - _localContentDb = BTree.GetTree(localContentDbPath, _localDbExists); - _localMediaDb = BTree.GetTree(localMediaDbPath, _localDbExists); + _localContentDb = BTree.GetTree(localContentDbPath, localContentDbExists); + _localMediaDb = BTree.GetTree(localMediaDbPath, localMediaDbExists); _logger.Info($"Registered with MainDom, local db exists? {_localDbExists}"); }, From e998fce5d1cecf1592f72c8bfb378af9060d3769 Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Thu, 17 Oct 2019 19:00:00 +0100 Subject: [PATCH 09/22] Trying to lock could throw exceptions so always make sure to properly clean up the local DB. --- .../PublishedCache/NuCache/ContentStore.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 1bd58c3878..3fe4b8aecd 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -234,11 +234,17 @@ namespace Umbraco.Web.PublishedCache.NuCache var lockInfo = new WriteLockInfo(); try { - Lock(lockInfo); - - if (_localDb == null) return; - _localDb.Dispose(); - _localDb = null; + try{ + // Trying to lock could throw exceptions so always make sure to clean up. + Lock(lockInfo); + } + catch + { + if (_localDb == null) return; + _localDb.Dispose(); + _localDb = null; + } + } finally { From ee098e019451cc3da11465e18f6b464598ebd64d Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Thu, 17 Oct 2019 19:06:28 +0100 Subject: [PATCH 10/22] Make sure the local dbs are disposed of if the content store was not created. --- .../PublishedCache/NuCache/PublishedSnapshotService.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index 81a3aa9e21..e08a0e3c52 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -144,8 +144,11 @@ namespace Umbraco.Web.PublishedCache.NuCache lock (_storesLock) { _contentStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned + // Make sure the local dbs are disposed of if the content store was not created. + _localContentDb?.Dispose(); _localContentDb = null; _mediaStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned + _localMediaDb?.Dispose(); _localMediaDb = null; } From 77f34e84665cced396a3924390bd3edc59eba62c Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Thu, 17 Oct 2019 19:32:21 +0100 Subject: [PATCH 11/22] Always reset the signal on a timeout exception waiting on the lock. --- src/Umbraco.Core/MainDom.cs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index ccecb4aa82..ae512241e1 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -180,17 +180,27 @@ namespace Umbraco.Core // if more than 1 instance reach that point, one will get the lock // and the other one will timeout, which is accepted - //TODO: This can throw a TimeoutException - in which case should this be in a try/finally to ensure the signal is always reset? - _systemLocker = _systemLock.Lock(LockTimeoutMilliseconds); + //This can throw a TimeoutException - in which case should this be in a try/finally to ensure the signal is always reset. + try + { + _systemLocker = _systemLock.Lock(LockTimeoutMilliseconds); + } + catch + { + throw; + } + finally + { + // we need to reset the event, because otherwise we would end up + // signaling ourselves and committing suicide immediately. + // only 1 instance can reach that point, but other instances may + // have started and be trying to get the lock - they will timeout, + // which is accepted + + _signal.Reset(); + } _isMainDom = true; - - // we need to reset the event, because otherwise we would end up - // signaling ourselves and committing suicide immediately. - // only 1 instance can reach that point, but other instances may - // have started and be trying to get the lock - they will timeout, - // which is accepted - - _signal.Reset(); + //WaitOneAsync (ext method) will wait for a signal without blocking the main thread, the waiting is done on a background thread From d774ff8bc15bd549caee73bc2b149b13df84d262 Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Fri, 18 Oct 2019 07:17:56 +0100 Subject: [PATCH 12/22] Undoing a bad previous change where logic in a catch should have been in a finally. Also dispose could throw an error so catch it and complete the clean up. --- .../PublishedCache/NuCache/ContentStore.cs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 3fe4b8aecd..179b262568 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -238,11 +238,21 @@ namespace Umbraco.Web.PublishedCache.NuCache // Trying to lock could throw exceptions so always make sure to clean up. Lock(lockInfo); } - catch + catch { throw; } + finally { - if (_localDb == null) return; - _localDb.Dispose(); - _localDb = null; + if (_localDb != null) + { + try + { + _localDb.Dispose(); + } + catch { /* TBD: May already be throwing so don't throw again */} + finally + { + _localDb = null; + } + } } } From 478bc708b958a9efc6a59b6f1e55aec53e8b1eab Mon Sep 17 00:00:00 2001 From: JohnBlair Date: Fri, 18 Oct 2019 07:31:15 +0100 Subject: [PATCH 13/22] Defensive programming around disposal of local content and media dbs. --- .../NuCache/PublishedSnapshotService.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index e08a0e3c52..d66bcd806b 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -145,10 +145,18 @@ namespace Umbraco.Web.PublishedCache.NuCache { _contentStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned // Make sure the local dbs are disposed of if the content store was not created. - _localContentDb?.Dispose(); + try + { + _localContentDb?.Dispose(); + } + catch { /* Carry on with cleanup */ } _localContentDb = null; _mediaStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned - _localMediaDb?.Dispose(); + try + { + _localMediaDb?.Dispose(); + } + catch { } _localMediaDb = null; } From b04f9c17ae74458b4203f9a875be03363cad1824 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 22 Oct 2019 11:55:05 +1100 Subject: [PATCH 14/22] Makes some updates based on code reviews --- src/Umbraco.Core/MainDom.cs | 6 +--- src/Umbraco.Core/Runtime/CoreRuntime.cs | 30 +++++++++---------- .../PublishedCache/NuCache/ContentStore.cs | 30 +++++++++---------- .../NuCache/PublishedSnapshotService.cs | 22 ++++++++------ 4 files changed, 43 insertions(+), 45 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index ae512241e1..79c6f08bd1 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -184,11 +184,7 @@ namespace Umbraco.Core try { _systemLocker = _systemLock.Lock(LockTimeoutMilliseconds); - } - catch - { - throw; - } + } finally { // we need to reset the event, because otherwise we would end up diff --git a/src/Umbraco.Core/Runtime/CoreRuntime.cs b/src/Umbraco.Core/Runtime/CoreRuntime.cs index 5cca74124d..5839ba6cfd 100644 --- a/src/Umbraco.Core/Runtime/CoreRuntime.cs +++ b/src/Umbraco.Core/Runtime/CoreRuntime.cs @@ -140,24 +140,24 @@ namespace Umbraco.Core.Runtime Compose(composition); // acquire the main domain - if this fails then anything that should be registered with MainDom will not operate - if (AcquireMainDom(mainDom)) - { - // determine our runtime level - DetermineRuntimeLevel(databaseFactory, ProfilingLogger); + AcquireMainDom(mainDom); - // get composers, and compose - var composerTypes = ResolveComposerTypes(typeLoader); - composition.WithCollectionBuilder(); - var composers = new Composers(composition, composerTypes, ProfilingLogger); - composers.Compose(); + // determine our runtime level + DetermineRuntimeLevel(databaseFactory, ProfilingLogger); - // create the factory - _factory = Current.Factory = composition.CreateFactory(); + // get composers, and compose + var composerTypes = ResolveComposerTypes(typeLoader); + composition.WithCollectionBuilder(); + var composers = new Composers(composition, composerTypes, ProfilingLogger); + composers.Compose(); + + // create the factory + _factory = Current.Factory = composition.CreateFactory(); + + // create & initialize the components + _components = _factory.GetInstance(); + _components.Initialize(); - // create & initialize the components - _components = _factory.GetInstance(); - _components.Initialize(); - } } catch (Exception e) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs index 179b262568..6cf34f68bb 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/ContentStore.cs @@ -128,7 +128,8 @@ namespace Umbraco.Web.PublishedCache.NuCache Monitor.Enter(_rlocko, ref rtaken); // see SnapDictionary - try { } finally + try { } + finally { _wlocked++; lockInfo.Count = true; @@ -234,27 +235,24 @@ namespace Umbraco.Web.PublishedCache.NuCache var lockInfo = new WriteLockInfo(); try { - try{ + try + { // Trying to lock could throw exceptions so always make sure to clean up. Lock(lockInfo); } - catch { throw; } - finally + finally { - if (_localDb != null) + try { - try - { - _localDb.Dispose(); - } - catch { /* TBD: May already be throwing so don't throw again */} - finally - { - _localDb = null; - } + _localDb?.Dispose(); + } + catch { /* TBD: May already be throwing so don't throw again */} + finally + { + _localDb = null; } } - + } finally { @@ -294,7 +292,7 @@ namespace Umbraco.Web.PublishedCache.NuCache public void UpdateContentTypes(IEnumerable types) { //nothing to do if this is empty, no need to lock/allocate/iterate/etc... - if (!types.Any()) return; + if (!types.Any()) return; var lockInfo = new WriteLockInfo(); try diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index d66bcd806b..d89ff47313 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -57,7 +57,8 @@ namespace Umbraco.Web.PublishedCache.NuCache private BPlusTree _localContentDb; private BPlusTree _localMediaDb; - private bool _localDbExists; + private bool _localContentDbExists; + private bool _localMediaDbExists; // define constant - determines whether to use cache when previewing // to store eg routes, property converted values, anything - caching @@ -128,14 +129,13 @@ namespace Umbraco.Web.PublishedCache.NuCache var path = GetLocalFilesPath(); var localContentDbPath = Path.Combine(path, "NuCache.Content.db"); var localMediaDbPath = Path.Combine(path, "NuCache.Media.db"); - var localContentDbExists = File.Exists(localContentDbPath); - var localMediaDbExists = File.Exists(localMediaDbPath); - _localDbExists = localContentDbExists && localMediaDbExists; + _localContentDbExists = File.Exists(localContentDbPath); + _localMediaDbExists = File.Exists(localMediaDbPath); // if both local databases exist then GetTree will open them, else new databases will be created - _localContentDb = BTree.GetTree(localContentDbPath, localContentDbExists); - _localMediaDb = BTree.GetTree(localMediaDbPath, localMediaDbExists); + _localContentDb = BTree.GetTree(localContentDbPath, _localContentDbExists); + _localMediaDb = BTree.GetTree(localMediaDbPath, _localMediaDbExists); - _logger.Info($"Registered with MainDom, local db exists? {_localDbExists}"); + _logger.Info($"Registered with MainDom, local content db exists? {_localContentDbExists}, local media db exists? {_localMediaDbExists}"); }, () => { @@ -200,11 +200,15 @@ namespace Umbraco.Web.PublishedCache.NuCache var okContent = false; var okMedia = false; - if (_localDbExists) + if (_localContentDbExists) { okContent = LockAndLoadContent(LoadContentFromLocalDbLocked); if (!okContent) - _logger.Warn("Loading content from local db raised warnings, will reload from database."); + _logger.Warn("Loading content from local db raised warnings, will reload from database."); + } + + if (_localMediaDbExists) + { okMedia = LockAndLoadMedia(LoadMediaFromLocalDbLocked); if (!okMedia) _logger.Warn("Loading media from local db raised warnings, will reload from database."); From 8a18a5b1cbe94180b7245cce5c145eeebdce347a Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 22 Oct 2019 12:13:40 +1100 Subject: [PATCH 15/22] ensure there is no casing issues with app physical path when generating a hash --- src/Umbraco.Core/MainDom.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 79c6f08bd1..2fda0b2cb0 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -66,7 +66,7 @@ namespace Umbraco.Core // we *cannot* use the process ID here because when an AppPool restarts it is // a new process for the same application path - var appPath = HostingEnvironment.ApplicationPhysicalPath; + var appPath = HostingEnvironment.ApplicationPhysicalPath.ToLowerInvariant(); var hash = (appId + ":::" + appPath).ToSHA1(); var lockName = "UMBRACO-" + hash + "-MAINDOM-LCK"; From fff646ad15c61c793fb42a16707ef25969ce4a6d Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 23 Oct 2019 14:12:23 +1100 Subject: [PATCH 16/22] MainDom will terminate on first (or only) call to Stop --- src/Umbraco.Core/MainDom.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 2fda0b2cb0..e54bd0dcb3 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -218,14 +218,10 @@ namespace Umbraco.Core { OnSignal("environment"); // will run once - // The web app is stopping, dispose eagerly + // The web app is stopping, need to wind down Dispose(true); - if (immediate) - { - //only unregister when it's the final call, else we won't be notified of the final call - HostingEnvironment.UnregisterObject(this); - } + HostingEnvironment.UnregisterObject(this); } #region IDisposable Support From 31ddc1d935a02169e8f27ed1ede0814a20788d40 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 23 Oct 2019 14:17:30 +1100 Subject: [PATCH 17/22] Changes BackgroundTaskRunner to shutdown faster and to ensures that any latched tasks are canceled even with Stop(immediate == false) is executed. --- .../Scheduling/BackgroundTaskRunner.cs | 64 +++++++++++-------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index dab6a8865e..e518f49ae0 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -338,14 +338,18 @@ namespace Umbraco.Web.Scheduling if (_isRunning == false) return; // done already } + var hasTasks = _tasks.Count > 0; + + if (!force && hasTasks) + _logger.Info("{LogPrefix} Waiting for tasks to complete", _logPrefix); + // complete the queue // will stop waiting on the queue or on a latch _tasks.Complete(); - if (force) + if (!hasTasks || force) { - // we must bring everything down, now - Thread.Sleep(100); // give time to Complete() + // we must bring everything down, now lock (_locker) { // was Complete() enough? @@ -354,13 +358,13 @@ namespace Umbraco.Web.Scheduling // try to cancel running async tasks (cannot do much about sync tasks) // break latched tasks // stop processing the queue - _shutdownTokenSource.Cancel(false); // false is the default - _shutdownTokenSource.Dispose(); + _shutdownTokenSource?.Cancel(false); // false is the default + _shutdownTokenSource?.Dispose(); _shutdownTokenSource = null; } // tasks in the queue will be executed... - if (wait == false) return; + if (!wait) return; _runningTask?.Wait(CancellationToken.None); // wait for whatever is running to end... } @@ -503,7 +507,7 @@ namespace Umbraco.Web.Scheduling // returns the task that completed // - latched.Latch completes when the latch releases // - _tasks.Completion completes when the runner completes - // - tokenTaskSource.Task completes when this task, or the whole runner, is cancelled + // - tokenTaskSource.Task completes when this task, or the whole runner is cancelled var task = await Task.WhenAny(latched.Latch, _tasks.Completion, tokenTaskSource.Task); // ok to run now @@ -693,13 +697,11 @@ namespace Umbraco.Web.Scheduling if (onTerminating) OnEvent(Terminating, "Terminating"); - if (immediate == false) + if (!immediate) { - // The Stop method is first called with the immediate parameter set to false. The object can either complete - // processing, call the UnregisterObject method, and then return or it can return immediately and complete - // processing asynchronously before calling the UnregisterObject method. - - _logger.Info("{LogPrefix} Waiting for tasks to complete", _logPrefix); + // immediate == false when the app is trying to wind down, immediate == true will be called either: + // after a call with immediate == false or if the app is not trying to wind down and needs to immediately stop. + // So Stop may be called twice or sometimes only once. try { @@ -716,24 +718,32 @@ namespace Umbraco.Web.Scheduling Terminate(false); } } - + + // If we are called with immediate == false, wind down above and then shutdown within 2 seconds, + // we want to shut down the app as quick as possible, if we wait until immediate == true, this can + // take a very long time since immediate will only be true when a new request is received on the new + // appdomain (or another iis timeout occurs ... which can take soeme time). + Task.Delay(2000, _shutdownToken).ContinueWith(_ => StopImmediate()); + } else { - // If the registered object does not complete processing before the application manager's time-out - // period expires, the Stop method is called again with the immediate parameter set to true. When the - // immediate parameter is true, the registered object must call the UnregisterObject method before returning; - // otherwise, its registration will be removed by the application manager. + // If we are called with immediate == true - cancel and shut down now. - _logger.Info("{LogPrefix} Canceling tasks", _logPrefix); - try - { - Shutdown(true, true); // cancel all tasks, wait for the current one to end - } - finally - { - Terminate(true); - } + StopImmediate(); + } + } + + private void StopImmediate() + { + _logger.Info("{LogPrefix} Canceling tasks", _logPrefix); + try + { + Shutdown(true, true); // cancel all tasks, wait for the current one to end + } + finally + { + Terminate(true); } } From 52b0edf1f50e1f2d4ef60c1a36b43d3edd15220e Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 24 Oct 2019 14:24:01 +1100 Subject: [PATCH 18/22] For backgroundtaskrunner, run shutdown on a threadpool thread to not block the shutdown of other IRegisteredObjects --- .../Scheduling/BackgroundTaskRunner.cs | 80 ++++++++++++------- 1 file changed, 53 insertions(+), 27 deletions(-) diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index e518f49ae0..403ffd47a8 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -697,43 +697,69 @@ namespace Umbraco.Web.Scheduling if (onTerminating) OnEvent(Terminating, "Terminating"); + // Run the Stop commands on another thread since IRegisteredObject.Stop calls are called sequentially + // with a single aspnet thread during shutdown and we don't want to delay other calls to IRegisteredObject.Stop. if (!immediate) { - // immediate == false when the app is trying to wind down, immediate == true will be called either: - // after a call with immediate == false or if the app is not trying to wind down and needs to immediately stop. - // So Stop may be called twice or sometimes only once. - - try - { - Shutdown(false, false); // do not accept any more tasks, flush the queue, do not wait - } - finally - { - // raise the completed event only after the running threading task has completed - lock (_locker) - { - if (_runningTask != null) - _runningTask.ContinueWith(_ => Terminate(false)); - else - Terminate(false); - } - } - - // If we are called with immediate == false, wind down above and then shutdown within 2 seconds, - // we want to shut down the app as quick as possible, if we wait until immediate == true, this can - // take a very long time since immediate will only be true when a new request is received on the new - // appdomain (or another iis timeout occurs ... which can take soeme time). - Task.Delay(2000, _shutdownToken).ContinueWith(_ => StopImmediate()); - + Task.Run(StopInitial, CancellationToken.None); } else { - // If we are called with immediate == true - cancel and shut down now. + lock(_locker) + { + if (_terminated) return; + Task.Run(StopImmediate, CancellationToken.None); + } + } + } + /// + /// Called when immediate == false for IRegisteredObject.Stop(bool immediate) + /// + /// + /// Called on a threadpool thread + /// + private void StopInitial() + { + // immediate == false when the app is trying to wind down, immediate == true will be called either: + // after a call with immediate == false or if the app is not trying to wind down and needs to immediately stop. + // So Stop may be called twice or sometimes only once. + + try + { + Shutdown(false, false); // do not accept any more tasks, flush the queue, do not wait + } + finally + { + // raise the completed event only after the running threading task has completed + lock (_locker) + { + if (_runningTask != null) + _runningTask.ContinueWith(_ => Terminate(false)); + else + Terminate(false); + } + } + + // If the shutdown token was not canceled in the Shutdown call above, it means there was still tasks + // being processed, in which case we'll give it a couple seconds + if (!_shutdownToken.IsCancellationRequested) + { + // If we are called with immediate == false, wind down above and then shutdown within 2 seconds, + // we want to shut down the app as quick as possible, if we wait until immediate == true, this can + // take a very long time since immediate will only be true when a new request is received on the new + // appdomain (or another iis timeout occurs ... which can take some time). + Thread.Sleep(2000); //we are already on a threadpool thread StopImmediate(); } } + /// + /// Called when immediate == true for IRegisteredObject.Stop(bool immediate) + /// + /// + /// Called on a threadpool thread + /// private void StopImmediate() { _logger.Info("{LogPrefix} Canceling tasks", _logPrefix); From f513ed547699ff3b2be931e547aaa515c4983aad Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 24 Oct 2019 14:35:30 +1100 Subject: [PATCH 19/22] Changes call transition from StopInitial to StopImmediate instead of terminate to ensure that all cancelation tokens are canceled and cleared --- src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index 403ffd47a8..4158594109 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -735,9 +735,9 @@ namespace Umbraco.Web.Scheduling lock (_locker) { if (_runningTask != null) - _runningTask.ContinueWith(_ => Terminate(false)); + _runningTask.ContinueWith(_ => StopImmediate()); else - Terminate(false); + StopImmediate(); } } From 6bba032325437d6de9bad16919f911e7dcae7aa4 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 4 Dec 2019 16:09:39 +1100 Subject: [PATCH 20/22] re-merges --- .../NuCache/PublishedSnapshotService.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index b40470e131..a866297d72 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -128,9 +128,9 @@ namespace Umbraco.Web.PublishedCache.NuCache // stores need to be populated, happens in OnResolutionFrozen which uses _localDbExists to // figure out whether it can read the databases or it should populate them from sql - _logger.Info("Creating the content store, localContentDbExists? {LocalContentDbExists}", _localDbExists); + _logger.Info("Creating the content store, localContentDbExists? {LocalContentDbExists}", _localContentDbExists); _contentStore = new ContentStore(publishedSnapshotAccessor, variationContextAccessor, logger, _localContentDb); - _logger.Info("Creating the media store, localMediaDbExists? {LocalMediaDbExists}", _localDbExists); + _logger.Info("Creating the media store, localMediaDbExists? {LocalMediaDbExists}", _localMediaDbExists); _mediaStore = new ContentStore(publishedSnapshotAccessor, variationContextAccessor, logger, _localMediaDb); } else @@ -171,14 +171,15 @@ namespace Umbraco.Web.PublishedCache.NuCache var path = GetLocalFilesPath(); var localContentDbPath = Path.Combine(path, "NuCache.Content.db"); var localMediaDbPath = Path.Combine(path, "NuCache.Media.db"); - var localContentDbExists = File.Exists(localContentDbPath); - var localMediaDbExists = File.Exists(localMediaDbPath); - _localDbExists = localContentDbExists && localMediaDbExists; - // if both local databases exist then GetTree will open them, else new databases will be created - _localContentDb = BTree.GetTree(localContentDbPath, _localDbExists); - _localMediaDb = BTree.GetTree(localMediaDbPath, _localDbExists); + + _localContentDbExists = File.Exists(localContentDbPath); + _localMediaDbExists = File.Exists(localMediaDbPath); - _logger.Info("Registered with MainDom, localContentDbExists? {LocalContentDbExists}, localMediaDbExists? {LocalMediaDbExists}", localContentDbExists, localMediaDbExists); + // if both local databases exist then GetTree will open them, else new databases will be created + _localContentDb = BTree.GetTree(localContentDbPath, _localContentDbExists); + _localMediaDb = BTree.GetTree(localMediaDbPath, _localMediaDbExists); + + _logger.Info("Registered with MainDom, localContentDbExists? {LocalContentDbExists}, localMediaDbExists? {LocalMediaDbExists}", _localContentDbExists, _localMediaDbExists); } /// @@ -211,11 +212,15 @@ namespace Umbraco.Web.PublishedCache.NuCache try { - if (_localDbExists) + if (_localContentDbExists) { okContent = LockAndLoadContent(scope => LoadContentFromLocalDbLocked(true)); if (!okContent) - _logger.Warn("Loading content from local db raised warnings, will reload from database."); + _logger.Warn("Loading content from local db raised warnings, will reload from database."); + } + + if (_localMediaDbExists) + { okMedia = LockAndLoadMedia(scope => LoadMediaFromLocalDbLocked(true)); if (!okMedia) _logger.Warn("Loading media from local db raised warnings, will reload from database."); From 6f9062c0cfcb1292fe40b594dc43b4644f622e83 Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 5 Dec 2019 19:19:09 +1100 Subject: [PATCH 21/22] fixes test (cherry picked) --- src/Umbraco.Core/MainDom.cs | 8 ++-- .../Scheduling/BackgroundTaskRunnerTests.cs | 43 +++++++----------- .../Scheduling/BackgroundTaskRunner.cs | 44 +++++++++++-------- 3 files changed, 45 insertions(+), 50 deletions(-) diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index 7adea7dc1a..a21afd6ffb 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -52,11 +52,9 @@ namespace Umbraco.Core _logger = logger; - var appId = string.Empty; // HostingEnvironment.ApplicationID is null in unit tests, making ReplaceNonAlphanumericChars fail - if (HostingEnvironment.ApplicationID != null) - appId = HostingEnvironment.ApplicationID.ReplaceNonAlphanumericChars(string.Empty); - + var appId = HostingEnvironment.ApplicationID?.ReplaceNonAlphanumericChars(string.Empty) ?? string.Empty; + // combining with the physical path because if running on eg IIS Express, // two sites could have the same appId even though they are different. // @@ -66,7 +64,7 @@ namespace Umbraco.Core // we *cannot* use the process ID here because when an AppPool restarts it is // a new process for the same application path - var appPath = HostingEnvironment.ApplicationPhysicalPath.ToLowerInvariant(); + var appPath = HostingEnvironment.ApplicationPhysicalPath?.ToLowerInvariant() ?? string.Empty; var hash = (appId + ":::" + appPath).GenerateHash(); var lockName = "UMBRACO-" + hash + "-MAINDOM-LCK"; diff --git a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs index 3664717af7..4d264c830e 100644 --- a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs +++ b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs @@ -183,25 +183,19 @@ namespace Umbraco.Tests.Scheduling runner.Terminated += (sender, args) => { terminated = true; }; Assert.IsFalse(runner.IsRunning); // because AutoStart is false - runner.Add(new MyTask(5000)); - runner.Add(new MyTask()); - runner.Add(t = new MyTask()); + runner.Add(new MyTask()); // sleeps 500 ms + runner.Add(new MyTask()); // sleeps 500 ms + runner.Add(t = new MyTask()); // sleeps 500 ms ... total = 1500 ms until it's done Assert.IsTrue(runner.IsRunning); // is running the task - runner.Stop(false); // -immediate = -force, -wait + await runner.StopInternal(false); // -immediate = -force, -wait (max 2000 ms delay before +immediate) + + Assert.IsTrue(stopped); // raised that one Assert.IsTrue(terminating); // has raised that event - Assert.IsFalse(terminated); // but not terminated yet + Assert.IsTrue(terminated); // and that event - // all this before we await because -wait Assert.IsTrue(runner.IsCompleted); // shutdown completes the runner - Assert.IsTrue(runner.IsRunning); // still running the task - - await runner.StoppedAwaitable; // runner stops, within test's timeout - Assert.IsFalse(runner.IsRunning); - Assert.IsTrue(stopped); - - await runner.TerminatedAwaitable; // runner terminates, within test's timeout - Assert.IsTrue(terminated); // has raised that event + Assert.IsFalse(runner.IsRunning); // done running Assert.AreNotEqual(DateTime.MinValue, t.Ended); // t has run } @@ -222,23 +216,20 @@ namespace Umbraco.Tests.Scheduling runner.Terminated += (sender, args) => { terminated = true; }; Assert.IsFalse(runner.IsRunning); // because AutoStart is false - runner.Add(new MyTask(5000)); - runner.Add(new MyTask()); - runner.Add(t = new MyTask()); + runner.Add(new MyTask()); // sleeps 500 ms + runner.Add(new MyTask()); // sleeps 500 ms + runner.Add(t = new MyTask()); // sleeps 500 ms ... total = 1500 ms until it's done Assert.IsTrue(runner.IsRunning); // is running the task - runner.Stop(true); // +immediate = +force, +wait + await runner.StopInternal(true); // +immediate = +force, +wait (no delay) + + Assert.IsTrue(stopped); // raised that one Assert.IsTrue(terminating); // has raised that event Assert.IsTrue(terminated); // and that event - Assert.IsTrue(stopped); // and that one - - // and all this before we await because +wait + Assert.IsTrue(runner.IsCompleted); // shutdown completes the runner Assert.IsFalse(runner.IsRunning); // done running - await runner.StoppedAwaitable; // runner stops, within test's timeout - await runner.TerminatedAwaitable; // runner terminates, within test's timeout - Assert.AreEqual(DateTime.MinValue, t.Ended); // t has *not* run } } @@ -564,7 +555,7 @@ namespace Umbraco.Tests.Scheduling Thread.Sleep(1000); Assert.IsTrue(runner.IsRunning); // still waiting for the task to release Assert.IsFalse(task.HasRun); - task.Release(); + task.Release(); // unlatch var runnerTask = runner.CurrentThreadingTask; // may be null if things go fast enough if (runnerTask != null) await runnerTask; // wait for current task to complete @@ -574,7 +565,7 @@ namespace Umbraco.Tests.Scheduling } [Test] - public async Task LatchedTaskStops() + public async Task LatchedTaskStops_Runs_On_Shutdown() { using (var runner = new BackgroundTaskRunner(new BackgroundTaskRunnerOptions(), _logger)) { diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index 4158594109..ebb9162f29 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -338,7 +338,7 @@ namespace Umbraco.Web.Scheduling if (_isRunning == false) return; // done already } - var hasTasks = _tasks.Count > 0; + var hasTasks = TaskCount > 0; if (!force && hasTasks) _logger.Info("{LogPrefix} Waiting for tasks to complete", _logPrefix); @@ -432,7 +432,7 @@ namespace Umbraco.Web.Scheduling lock (_locker) { // deal with race condition - if (_shutdownToken.IsCancellationRequested == false && _tasks.Count > 0) continue; + if (_shutdownToken.IsCancellationRequested == false && TaskCount > 0) continue; // if we really have nothing to do, stop _logger.Debug("{LogPrefix} Stopping", _logPrefix); @@ -457,7 +457,7 @@ namespace Umbraco.Web.Scheduling // if KeepAlive is false then don't block, exit if there is // no task in the buffer - yes, there is a race condition, which // we'll take care of - if (_options.KeepAlive == false && _tasks.Count == 0) + if (_options.KeepAlive == false && TaskCount == 0) return null; try @@ -514,8 +514,12 @@ namespace Umbraco.Web.Scheduling if (task == latched.Latch) return bgTask; + // we are shutting down if the _tasks.Complete(); was called or the shutdown token was cancelled + var isShuttingDown = _shutdownToken.IsCancellationRequested || task == _tasks.Completion; + // if shutting down, return the task only if it runs on shutdown - if (_shutdownToken.IsCancellationRequested == false && latched.RunsOnShutdown) return bgTask; + if (isShuttingDown && latched.RunsOnShutdown) + return bgTask; // else, either it does not run on shutdown or it's been cancelled, dispose latched.Dispose(); @@ -668,17 +672,7 @@ namespace Umbraco.Web.Scheduling #endregion - /// - /// Requests a registered object to un-register. - /// - /// true to indicate the registered object should un-register from the hosting - /// environment before returning; otherwise, false. - /// - /// "When the application manager needs to stop a registered object, it will call the Stop method." - /// The application manager will call the Stop method to ask a registered object to un-register. During - /// processing of the Stop method, the registered object must call the HostingEnvironment.UnregisterObject method. - /// - public void Stop(bool immediate) + internal Task StopInternal(bool immediate) { // the first time the hosting environment requests that the runner terminates, // raise the Terminating event - that could be used to prevent any process that @@ -701,18 +695,30 @@ namespace Umbraco.Web.Scheduling // with a single aspnet thread during shutdown and we don't want to delay other calls to IRegisteredObject.Stop. if (!immediate) { - Task.Run(StopInitial, CancellationToken.None); + return Task.Run(StopInitial, CancellationToken.None); } else { - lock(_locker) + lock (_locker) { - if (_terminated) return; - Task.Run(StopImmediate, CancellationToken.None); + if (_terminated) return Task.FromResult(0); + return Task.Run(StopImmediate, CancellationToken.None); } } } + /// + /// Requests a registered object to un-register. + /// + /// true to indicate the registered object should un-register from the hosting + /// environment before returning; otherwise, false. + /// + /// "When the application manager needs to stop a registered object, it will call the Stop method." + /// The application manager will call the Stop method to ask a registered object to un-register. During + /// processing of the Stop method, the registered object must call the HostingEnvironment.UnregisterObject method. + /// + public void Stop(bool immediate) => StopInternal(immediate); + /// /// Called when immediate == false for IRegisteredObject.Stop(bool immediate) /// From 3f78331145fee09e763e409b42e307ee542cab6e Mon Sep 17 00:00:00 2001 From: Shannon Date: Thu, 5 Dec 2019 23:26:49 +1100 Subject: [PATCH 22/22] Adds lots of notes, fixes tests to ensure we catch OperationCanceledException where necessary, adds logging to tests, fixes a boolean op --- .../Scheduling/BackgroundTaskRunnerTests.cs | 63 ++++++++++++------- .../Scheduling/BackgroundTaskRunner.cs | 33 +++++++--- 2 files changed, 64 insertions(+), 32 deletions(-) diff --git a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs index 4d264c830e..ea42cd2ea9 100644 --- a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs +++ b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs @@ -7,6 +7,7 @@ using System.Threading.Tasks; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Logging; +using Umbraco.Tests.TestHelpers; using Umbraco.Web.Scheduling; namespace Umbraco.Tests.Scheduling @@ -21,7 +22,7 @@ namespace Umbraco.Tests.Scheduling [OneTimeSetUp] public void InitializeFixture() { - _logger = new DebugDiagnosticsLogger(); + _logger = new ConsoleLogger(); } [Test] @@ -102,12 +103,12 @@ namespace Umbraco.Tests.Scheduling { using (var runner = new BackgroundTaskRunner(new BackgroundTaskRunnerOptions(), _logger)) { - MyTask t; + MyTask t1, t2, t3; Assert.IsFalse(runner.IsRunning); // because AutoStart is false - runner.Add(new MyTask(5000)); - runner.Add(new MyTask()); - runner.Add(t = new MyTask()); + runner.Add(t1 = new MyTask(5000)); + runner.Add(t2 = new MyTask()); + runner.Add(t3 = new MyTask()); Assert.IsTrue(runner.IsRunning); // is running tasks // shutdown -force => run all queued tasks @@ -115,7 +116,7 @@ namespace Umbraco.Tests.Scheduling Assert.IsTrue(runner.IsRunning); // is running tasks await runner.StoppedAwaitable; // runner stops, within test's timeout - Assert.AreNotEqual(DateTime.MinValue, t.Ended); // t has run + Assert.AreNotEqual(DateTime.MinValue, t3.Ended); // t3 has run } } @@ -124,20 +125,25 @@ namespace Umbraco.Tests.Scheduling { using (var runner = new BackgroundTaskRunner(new BackgroundTaskRunnerOptions(), _logger)) { - MyTask t; + MyTask t1, t2, t3; Assert.IsFalse(runner.IsRunning); // because AutoStart is false - runner.Add(new MyTask(5000)); - runner.Add(new MyTask()); - runner.Add(t = new MyTask()); + runner.Add(t1 = new MyTask(5000)); + runner.Add(t2 = new MyTask()); + runner.Add(t3 = new MyTask()); Assert.IsTrue(runner.IsRunning); // is running tasks + Thread.Sleep(1000); // since we are forcing shutdown, we need to give it a chance to start, else it will be canceled before the queue is started + // shutdown +force => tries to cancel the current task, ignores queued tasks runner.Shutdown(true, false); // +force -wait Assert.IsTrue(runner.IsRunning); // is running that long task it cannot cancel - await runner.StoppedAwaitable; // runner stops, within test's timeout - Assert.AreEqual(DateTime.MinValue, t.Ended); // t has *not* run + await runner.StoppedAwaitable; // runner stops, within test's timeout (no cancelation token used, no need to catch OperationCanceledException) + + Assert.AreNotEqual(DateTime.MinValue, t1.Ended); // t1 *has* run + Assert.AreEqual(DateTime.MinValue, t2.Ended); // t2 has *not* run + Assert.AreEqual(DateTime.MinValue, t3.Ended); // t3 has *not* run } } @@ -163,7 +169,15 @@ namespace Umbraco.Tests.Scheduling // shutdown +force => tries to cancel the current task, ignores queued tasks runner.Shutdown(true, false); // +force -wait - await runner.StoppedAwaitable; // runner stops, within test's timeout + try + { + await runner.StoppedAwaitable; // runner stops, within test's timeout ... maybe + } + catch (OperationCanceledException) + { + // catch exception, this can occur because we are +force shutting down which will + // cancel a pending task if the queue hasn't completed in time + } } } @@ -188,7 +202,8 @@ namespace Umbraco.Tests.Scheduling runner.Add(t = new MyTask()); // sleeps 500 ms ... total = 1500 ms until it's done Assert.IsTrue(runner.IsRunning); // is running the task - await runner.StopInternal(false); // -immediate = -force, -wait (max 2000 ms delay before +immediate) + runner.Stop(false); // -immediate = -force, -wait (max 2000 ms delay before +immediate) + await runner.TerminatedAwaitable; Assert.IsTrue(stopped); // raised that one Assert.IsTrue(terminating); // has raised that event @@ -221,7 +236,8 @@ namespace Umbraco.Tests.Scheduling runner.Add(t = new MyTask()); // sleeps 500 ms ... total = 1500 ms until it's done Assert.IsTrue(runner.IsRunning); // is running the task - await runner.StopInternal(true); // +immediate = +force, +wait (no delay) + runner.Stop(true); // +immediate = +force, +wait (no delay) + await runner.TerminatedAwaitable; Assert.IsTrue(stopped); // raised that one Assert.IsTrue(terminating); // has raised that event @@ -255,8 +271,7 @@ namespace Umbraco.Tests.Scheduling }, _logger)) { Assert.IsTrue(runner.IsRunning); // because AutoStart is true - runner.Stop(false); // keepalive = must be stopped - await runner.StoppedAwaitable; // runner stops, within test's timeout + await runner.StopInternal(false); // keepalive = must be stopped } } @@ -282,13 +297,9 @@ namespace Umbraco.Tests.Scheduling // dispose will stop it } - await runner.StoppedAwaitable; // runner stops, within test's timeout - //await runner.TerminatedAwaitable; // NO! see note below + await runner.StoppedAwaitable; Assert.Throws(() => runner.Add(new MyTask())); - // but do NOT await on TerminatedAwaitable - disposing just shuts the runner down - // so that we don't have a runaway task in tests, etc - but it does NOT terminate - // the runner - it really is NOT a nice way to end a runner - it's there for tests } [Test] @@ -575,7 +586,7 @@ namespace Umbraco.Tests.Scheduling Thread.Sleep(5000); Assert.IsTrue(runner.IsRunning); // still waiting for the task to release Assert.IsFalse(task.HasRun); - runner.Shutdown(false, false); + runner.Shutdown(false, false); // -force, -wait await runner.StoppedAwaitable; // wait for the entire runner operation to complete Assert.IsTrue(task.HasRun); } @@ -871,7 +882,9 @@ namespace Umbraco.Tests.Scheduling public override void PerformRun() { + Console.WriteLine($"Sleeping {_milliseconds}..."); Thread.Sleep(_milliseconds); + Console.WriteLine("Wake up!"); } } @@ -988,7 +1001,9 @@ namespace Umbraco.Tests.Scheduling public DateTime Ended { get; set; } public virtual void Dispose() - { } + { + + } } } } diff --git a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs index ebb9162f29..c0475b1f79 100644 --- a/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs +++ b/src/Umbraco.Web/Scheduling/BackgroundTaskRunner.cs @@ -199,7 +199,7 @@ namespace Umbraco.Web.Scheduling { lock (_locker) { - var task = _runningTask ?? Task.FromResult(0); + var task = _runningTask ?? Task.CompletedTask; return new ThreadingTaskImmutable(task); } } @@ -211,8 +211,9 @@ namespace Umbraco.Web.Scheduling /// An awaitable object. /// /// Used to wait until the runner has terminated. - /// This is for unit tests and should not be used otherwise. In most cases when the runner - /// has terminated, the application domain is going down and it is not the right time to do things. + /// + /// The only time the runner will be terminated is by the Hosting Environment when the application is being shutdown. + /// /// internal ThreadingTaskImmutable TerminatedAwaitable { @@ -347,14 +348,18 @@ namespace Umbraco.Web.Scheduling // will stop waiting on the queue or on a latch _tasks.Complete(); - if (!hasTasks || force) + if (force) { // we must bring everything down, now lock (_locker) { // was Complete() enough? - if (_isRunning == false) return; + // if _tasks.Complete() ended up triggering code to stop the runner and reset + // the _isRunning flag, then there's no need to initiate a cancel on the cancelation token. + if (_isRunning == false) + return; } + // try to cancel running async tasks (cannot do much about sync tasks) // break latched tasks // stop processing the queue @@ -586,17 +591,18 @@ namespace Umbraco.Web.Scheduling // triggers when the hosting environment requests that the runner terminates internal event TypedEventHandler, EventArgs> Terminating; - // triggers when the runner has terminated (no task can be added, no task is running) + // triggers when the hosting environment has terminated (no task can be added, no task is running) internal event TypedEventHandler, EventArgs> Terminated; private void OnEvent(TypedEventHandler, EventArgs> handler, string name) { - if (handler == null) return; OnEvent(handler, name, EventArgs.Empty); } private void OnEvent(TypedEventHandler, TArgs> handler, string name, TArgs e) { + _logger.Debug("{LogPrefix} OnEvent {EventName}", _logPrefix, name); + if (handler == null) return; try @@ -672,6 +678,15 @@ namespace Umbraco.Web.Scheduling #endregion + #region IRegisteredObject.Stop + + /// + /// Used by IRegisteredObject.Stop and shutdown on threadpool threads to not block shutdown times. + /// + /// + /// + /// An awaitable Task that is used to handle the shutdown. + /// internal Task StopInternal(bool immediate) { // the first time the hosting environment requests that the runner terminates, @@ -701,7 +716,7 @@ namespace Umbraco.Web.Scheduling { lock (_locker) { - if (_terminated) return Task.FromResult(0); + if (_terminated) return Task.CompletedTask; return Task.Run(StopImmediate, CancellationToken.None); } } @@ -810,5 +825,7 @@ namespace Umbraco.Web.Scheduling terminatedSource.TrySetResult(0); } + + #endregion } }