From 36030904212d321af75f96bfc894742e86629ec3 Mon Sep 17 00:00:00 2001 From: Shannon Date: Wed, 17 Jul 2019 22:51:14 +1000 Subject: [PATCH] Fixes race condition in PublishedSnapshotService and more... --- src/Umbraco.Core/AsyncLock.cs | 47 ++++++++-------- src/Umbraco.Core/MainDom.cs | 7 ++- src/Umbraco.Core/Runtime/CoreRuntime.cs | 6 +- src/Umbraco.Core/RuntimeState.cs | 5 ++ src/Umbraco.Core/WaitHandleExtensions.cs | 2 + .../PublishedContent/NuCacheChildrenTests.cs | 2 +- .../PublishedContent/NuCacheTests.cs | 2 +- .../Scoping/ScopedNuCacheTests.cs | 2 +- .../ContentTypeServiceVariantsTests.cs | 2 +- .../PublishedCache/NuCache/NuCacheComposer.cs | 2 +- .../NuCache/PublishedSnapshotService.cs | 56 +++++++++---------- .../PublishedSnapshotServiceOptions.cs | 28 ++++++++++ src/Umbraco.Web/Scheduling/KeepAlive.cs | 2 +- src/Umbraco.Web/Umbraco.Web.csproj | 1 + 14 files changed, 101 insertions(+), 63 deletions(-) create mode 100644 src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotServiceOptions.cs diff --git a/src/Umbraco.Core/AsyncLock.cs b/src/Umbraco.Core/AsyncLock.cs index 158b132f26..6dd866705e 100644 --- a/src/Umbraco.Core/AsyncLock.cs +++ b/src/Umbraco.Core/AsyncLock.cs @@ -67,31 +67,34 @@ namespace Umbraco.Core : new NamedSemaphoreReleaser(_semaphore2); } - public Task LockAsync() - { - var wait = _semaphore != null - ? _semaphore.WaitAsync() - : _semaphore2.WaitOneAsync(); + //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 - 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() + //{ + // var wait = _semaphore != null + // ? _semaphore.WaitAsync() + // : _semaphore2.WaitOneAsync(); - 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); + //} - 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() { diff --git a/src/Umbraco.Core/MainDom.cs b/src/Umbraco.Core/MainDom.cs index ca875c2167..d1012fb669 100644 --- a/src/Umbraco.Core/MainDom.cs +++ b/src/Umbraco.Core/MainDom.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Web.Hosting; @@ -113,7 +114,7 @@ namespace Umbraco.Core lock (_locko) { - _logger.Debug("Signaled {Signaled} ({SignalSource})", _signaled ? "(again)" : string.Empty, source); + _logger.Debug("Signaled ({Signaled}) ({SignalSource})", _signaled ? "again" : "first", source); if (_signaled) return; if (_isMainDom == false) return; // probably not needed _signaled = true; @@ -171,6 +172,7 @@ 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? _asyncLocker = _asyncLock.Lock(LockTimeoutMilliseconds); _isMainDom = true; @@ -181,6 +183,9 @@ namespace Umbraco.Core // 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 + _signal.WaitOneAsync() .ContinueWith(_ => OnSignal("signal")); diff --git a/src/Umbraco.Core/Runtime/CoreRuntime.cs b/src/Umbraco.Core/Runtime/CoreRuntime.cs index f9a41b4f66..5b069641c4 100644 --- a/src/Umbraco.Core/Runtime/CoreRuntime.cs +++ b/src/Umbraco.Core/Runtime/CoreRuntime.cs @@ -139,7 +139,7 @@ namespace Umbraco.Core.Runtime // there should be none, really - this is here "just in case" Compose(composition); - // acquire the main domain + // acquire the main domain - if this fails then anything that should be registered with MainDom will not operate AcquireMainDom(mainDom); // determine our runtime level @@ -218,13 +218,13 @@ namespace Umbraco.Core.Runtime IOHelper.SetRootDirectory(path); } - private void AcquireMainDom(MainDom mainDom) + private bool AcquireMainDom(MainDom mainDom) { using (var timer = ProfilingLogger.DebugDuration("Acquiring MainDom.", "Acquired.")) { try { - mainDom.Acquire(); + return mainDom.Acquire(); } catch { diff --git a/src/Umbraco.Core/RuntimeState.cs b/src/Umbraco.Core/RuntimeState.cs index 6fb8a04c0d..5d34fe70a1 100644 --- a/src/Umbraco.Core/RuntimeState.cs +++ b/src/Umbraco.Core/RuntimeState.cs @@ -97,6 +97,11 @@ namespace Umbraco.Core /// internal void EnsureApplicationUrl(HttpRequestBase request = null) { + //Fixme: This causes problems with site swap on azure because azure pre-warms a site by calling into `localhost` and when it does that + // it changes the URL to `localhost:80` which actually doesn't work for pinging itself, it only works internally in Azure. The ironic part + // about this is that this is here specifically for the slot swap scenario https://issues.umbraco.org/issue/U4-10626 + + // see U4-10626 - in some cases we want to reset the application url // (this is a simplified version of what was in 7.x) // note: should this be optional? is it expensive? diff --git a/src/Umbraco.Core/WaitHandleExtensions.cs b/src/Umbraco.Core/WaitHandleExtensions.cs index 0d840a2496..7a9294c113 100644 --- a/src/Umbraco.Core/WaitHandleExtensions.cs +++ b/src/Umbraco.Core/WaitHandleExtensions.cs @@ -23,6 +23,8 @@ namespace Umbraco.Core handle, (state, timedOut) => { + //TODO: We aren't checking if this is timed out + tcs.SetResult(null); // we take a lock here to make sure the outer method has completed setting the local variable callbackHandle. diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs index f3a520ead1..55304d257b 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheChildrenTests.cs @@ -124,7 +124,7 @@ namespace Umbraco.Tests.PublishedContent _source = new TestDataSource(kits); // at last, create the complete NuCache snapshot service! - var options = new PublishedSnapshotService.Options { IgnoreLocalDb = true }; + var options = new PublishedSnapshotServiceOptions { IgnoreLocalDb = true }; _snapshotService = new PublishedSnapshotService(options, null, runtime, diff --git a/src/Umbraco.Tests/PublishedContent/NuCacheTests.cs b/src/Umbraco.Tests/PublishedContent/NuCacheTests.cs index b66404c954..399b0c1342 100644 --- a/src/Umbraco.Tests/PublishedContent/NuCacheTests.cs +++ b/src/Umbraco.Tests/PublishedContent/NuCacheTests.cs @@ -169,7 +169,7 @@ namespace Umbraco.Tests.PublishedContent _variationAccesor = new TestVariationContextAccessor(); // at last, create the complete NuCache snapshot service! - var options = new PublishedSnapshotService.Options { IgnoreLocalDb = true }; + var options = new PublishedSnapshotServiceOptions { IgnoreLocalDb = true }; _snapshotService = new PublishedSnapshotService(options, null, runtime, diff --git a/src/Umbraco.Tests/Scoping/ScopedNuCacheTests.cs b/src/Umbraco.Tests/Scoping/ScopedNuCacheTests.cs index 397a22fc62..c974fed99e 100644 --- a/src/Umbraco.Tests/Scoping/ScopedNuCacheTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopedNuCacheTests.cs @@ -72,7 +72,7 @@ namespace Umbraco.Tests.Scoping protected override IPublishedSnapshotService CreatePublishedSnapshotService() { - var options = new PublishedSnapshotService.Options { IgnoreLocalDb = true }; + var options = new PublishedSnapshotServiceOptions { IgnoreLocalDb = true }; var publishedSnapshotAccessor = new UmbracoContextPublishedSnapshotAccessor(Umbraco.Web.Composing.Current.UmbracoContextAccessor); var runtimeStateMock = new Mock(); runtimeStateMock.Setup(x => x.Level).Returns(() => RuntimeLevel.Run); diff --git a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs index 3121988bfe..18ea95cd98 100644 --- a/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs +++ b/src/Umbraco.Tests/Services/ContentTypeServiceVariantsTests.cs @@ -42,7 +42,7 @@ namespace Umbraco.Tests.Services protected override IPublishedSnapshotService CreatePublishedSnapshotService() { - var options = new PublishedSnapshotService.Options { IgnoreLocalDb = true }; + var options = new PublishedSnapshotServiceOptions { IgnoreLocalDb = true }; var publishedSnapshotAccessor = new UmbracoContextPublishedSnapshotAccessor(Umbraco.Web.Composing.Current.UmbracoContextAccessor); var runtimeStateMock = new Mock(); runtimeStateMock.Setup(x => x.Level).Returns(() => RuntimeLevel.Run); diff --git a/src/Umbraco.Web/PublishedCache/NuCache/NuCacheComposer.cs b/src/Umbraco.Web/PublishedCache/NuCache/NuCacheComposer.cs index 72fa39e7e4..f748fd555c 100644 --- a/src/Umbraco.Web/PublishedCache/NuCache/NuCacheComposer.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/NuCacheComposer.cs @@ -15,7 +15,7 @@ namespace Umbraco.Web.PublishedCache.NuCache // register the NuCache published snapshot service // must register default options, required in the service ctor - composition.Register(factory => new PublishedSnapshotService.Options()); + composition.Register(factory => new PublishedSnapshotServiceOptions()); composition.SetPublishedSnapshotService(); // add the NuCache health check (hidden from type finder) diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs index dad9811af8..5e7c0542db 100755 --- a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotService.cs @@ -30,7 +30,8 @@ using File = System.IO.File; namespace Umbraco.Web.PublishedCache.NuCache { - class PublishedSnapshotService : PublishedSnapshotServiceBase + + internal class PublishedSnapshotService : PublishedSnapshotServiceBase { private readonly ServiceContext _serviceContext; private readonly IPublishedContentTypeFactory _publishedContentTypeFactory; @@ -56,7 +57,7 @@ namespace Umbraco.Web.PublishedCache.NuCache private BPlusTree _localContentDb; private BPlusTree _localMediaDb; - private readonly bool _localDbExists; + private bool _localDbExists; // define constant - determines whether to use cache when previewing // to store eg routes, property converted values, anything - caching @@ -68,7 +69,7 @@ namespace Umbraco.Web.PublishedCache.NuCache //private static int _singletonCheck; - public PublishedSnapshotService(Options options, IMainDom mainDom, IRuntimeState runtime, + public PublishedSnapshotService(PublishedSnapshotServiceOptions options, IMainDom mainDom, IRuntimeState runtime, ServiceContext serviceContext, IPublishedContentTypeFactory publishedContentTypeFactory, IdkMap idkMap, IPublishedSnapshotAccessor publishedSnapshotAccessor, IVariationContextAccessor variationContextAccessor, ILogger logger, IScopeProvider scopeProvider, IDocumentRepository documentRepository, IMediaRepository mediaRepository, IMemberRepository memberRepository, @@ -115,30 +116,36 @@ namespace Umbraco.Web.PublishedCache.NuCache if (options.IgnoreLocalDb == false) { var registered = mainDom.Register( - null, () => { + //"install" phase of MainDom + //this is inside of a lock in MainDom so this is guaranteed to run if MainDom was acquired and guaranteed + //to not run if MainDom wasn't acquired. + //If MainDom was not acquired, then _localContentDb and _localMediaDb will remain null which means this appdomain + //will load in published content via the DB and in that case this appdomain will probably not exist long enough to + //serve more than a page of content. + + 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); + // 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); + }, + () => + { + //"release" phase of MainDom + lock (_storesLock) { - _contentStore.ReleaseLocalDb(); + _contentStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned _localContentDb = null; - _mediaStore.ReleaseLocalDb(); + _mediaStore?.ReleaseLocalDb(); //null check because we could shut down before being assigned _localMediaDb = null; } }); - if (registered) - { - var path = GetLocalFilesPath(); - var localContentDbPath = Path.Combine(path, "NuCache.Content.db"); - var localMediaDbPath = Path.Combine(path, "NuCache.Media.db"); - _localDbExists = System.IO.File.Exists(localContentDbPath) && System.IO.File.Exists(localMediaDbPath); - - // 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); - } - // stores are created with a db so they can write to it, but they do not read from it, // 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 @@ -251,19 +258,6 @@ namespace Umbraco.Web.PublishedCache.NuCache base.Dispose(); } - public class Options - { - // disabled: prevents the published snapshot from updating and exposing changes - // or even creating a new published snapshot to see changes, uses old cache = bad - // - //// indicates that the snapshot cache should reuse the application request cache - //// otherwise a new cache object would be created for the snapshot specifically, - //// which is the default - web boot manager uses this to optimize facades - //public bool PublishedSnapshotCacheIsApplicationRequestCache; - - public bool IgnoreLocalDb; - } - #endregion #region Local files diff --git a/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotServiceOptions.cs b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotServiceOptions.cs new file mode 100644 index 0000000000..ef9d83a802 --- /dev/null +++ b/src/Umbraco.Web/PublishedCache/NuCache/PublishedSnapshotServiceOptions.cs @@ -0,0 +1,28 @@ +namespace Umbraco.Web.PublishedCache.NuCache +{ + /// + /// Options class for configuring the + /// + public class PublishedSnapshotServiceOptions + { + // disabled: prevents the published snapshot from updating and exposing changes + // or even creating a new published snapshot to see changes, uses old cache = bad + // + //// indicates that the snapshot cache should reuse the application request cache + //// otherwise a new cache object would be created for the snapshot specifically, + //// which is the default - web boot manager uses this to optimize facades + //public bool PublishedSnapshotCacheIsApplicationRequestCache; + + + /// + /// If true this disables the persisted local cache files for content and media + /// + /// + /// By default this is false which means umbraco will use locally persisted cache files for reading in all published content and media on application startup. + /// The reason for this is to improve startup times because the alternative to populating the published content and media on application startup is to read + /// these values from the database. In scenarios where sites are relatively small (below a few thousand nodes) reading the content/media from the database to populate + /// the in memory cache isn't that slow and is only marginally slower than reading from the locally persisted cache files. + /// + public bool IgnoreLocalDb { get; set; } + } +} diff --git a/src/Umbraco.Web/Scheduling/KeepAlive.cs b/src/Umbraco.Web/Scheduling/KeepAlive.cs index 6dd7572b87..c07430df04 100644 --- a/src/Umbraco.Web/Scheduling/KeepAlive.cs +++ b/src/Umbraco.Web/Scheduling/KeepAlive.cs @@ -64,7 +64,7 @@ namespace Umbraco.Web.Scheduling } catch (Exception ex) { - _logger.Error(ex, "Failed (at '{UmbracoAppUrl}').", umbracoAppUrl); + _logger.Error(ex, "Keep alive failed (at '{UmbracoAppUrl}').", umbracoAppUrl); } } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 41a3393fa4..331b2ac7d0 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -224,6 +224,7 @@ +