From c761fa0506272a8321e32fd5fd927b2b4e2b3063 Mon Sep 17 00:00:00 2001 From: Shannon Date: Mon, 21 Dec 2020 17:41:12 +1100 Subject: [PATCH] New IPublishedSnapshotStatus, reduces IPublishedSnapshotService --- .../IPublishedSnapshotService.cs | 12 ++- .../IPublishedSnapshotStatus.cs | 18 ++++ .../ContentStore.cs | 11 +++ .../NuCacheComposer.cs | 4 + .../Persistence/NuCacheContentService.cs | 18 +++- .../PublishedSnapshotService.cs | 83 ++----------------- .../PublishedSnapshotStatus.cs | 51 ++++++++++++ .../XmlPublishedSnapshotService.cs | 10 +-- .../PublishedSnapshotCacheStatusController.cs | 42 ++++++---- .../Controllers/PublishedStatusController.cs | 14 ++-- 10 files changed, 146 insertions(+), 117 deletions(-) create mode 100644 src/Umbraco.Core/PublishedCache/IPublishedSnapshotStatus.cs create mode 100644 src/Umbraco.PublishedCache.NuCache/PublishedSnapshotStatus.cs diff --git a/src/Umbraco.Core/PublishedCache/IPublishedSnapshotService.cs b/src/Umbraco.Core/PublishedCache/IPublishedSnapshotService.cs index 840b64c541..ce61dc4b8e 100644 --- a/src/Umbraco.Core/PublishedCache/IPublishedSnapshotService.cs +++ b/src/Umbraco.Core/PublishedCache/IPublishedSnapshotService.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Threading.Tasks; using Umbraco.Web.Cache; namespace Umbraco.Web.PublishedCache @@ -102,12 +103,9 @@ namespace Umbraco.Web.PublishedCache /// The changes. void Notify(DomainCacheRefresher.JsonPayload[] payloads); - // TODO: This is weird, why is this is this a thing? Maybe IPublishedSnapshotStatus? - string GetStatus(); - - // TODO: This is weird, why is this is this a thing? Maybe IPublishedSnapshotStatus? - string StatusUrl { get; } - - void Collect(); + /// + /// Cleans up unused snapshots + /// + Task CollectAsync(); } } diff --git a/src/Umbraco.Core/PublishedCache/IPublishedSnapshotStatus.cs b/src/Umbraco.Core/PublishedCache/IPublishedSnapshotStatus.cs new file mode 100644 index 0000000000..0f88bd4085 --- /dev/null +++ b/src/Umbraco.Core/PublishedCache/IPublishedSnapshotStatus.cs @@ -0,0 +1,18 @@ +namespace Umbraco.Web.PublishedCache +{ + /// + /// Returns the currents status for nucache + /// + public interface IPublishedSnapshotStatus + { + /// + /// Gets the status report as a string + /// + string GetStatus(); + + /// + /// Gets the URL used to retreive the status + /// + string StatusUrl { get; } + } +} diff --git a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs index e79c195b46..bb03693adf 100644 --- a/src/Umbraco.PublishedCache.NuCache/ContentStore.cs +++ b/src/Umbraco.PublishedCache.NuCache/ContentStore.cs @@ -1352,7 +1352,9 @@ namespace Umbraco.Web.PublishedCache.NuCache // reading _floorGen is safe if _collectTask is null if (_collectTask == null && _collectAuto && _liveGen - _floorGen > CollectMinGenDelta) + { CollectAsyncLocked(); + } return snapshot; } @@ -1374,8 +1376,17 @@ namespace Umbraco.Web.PublishedCache.NuCache private Task CollectAsyncLocked() { + // NOTE: What in the heck is going on here? Why is any of this running in async contexts? + // SD: From what I can tell this was designed to be a set and forget background task to do the + // collecting which is why it's called from non-async methods within this class. This is + // slightly dangerous because it's not taking into account app shutdown. + // TODO: There should be a different method or class responsible for executing the cleanup on a + // background (set and forget) thread. + if (_collectTask != null) + { return _collectTask; + } // ReSharper disable InconsistentlySynchronizedField var task = _collectTask = Task.Run((Action)Collect); diff --git a/src/Umbraco.PublishedCache.NuCache/NuCacheComposer.cs b/src/Umbraco.PublishedCache.NuCache/NuCacheComposer.cs index 5e618d361c..c1c80cf43c 100644 --- a/src/Umbraco.PublishedCache.NuCache/NuCacheComposer.cs +++ b/src/Umbraco.PublishedCache.NuCache/NuCacheComposer.cs @@ -27,6 +27,10 @@ namespace Umbraco.Web.PublishedCache.NuCache builder.Services.AddTransient(factory => new PublishedSnapshotServiceOptions()); builder.SetPublishedSnapshotService(); + // Add as itself + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + // replace this service since we want to improve the content/media // mapping lookups if we are using nucache. // TODO: Gotta wonder how much this does actually improve perf? It's a lot of weird code to make this happen so hope it's worth it diff --git a/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs b/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs index 5c7fdeb53b..00c3671217 100644 --- a/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs +++ b/src/Umbraco.PublishedCache.NuCache/Persistence/NuCacheContentService.cs @@ -94,14 +94,26 @@ namespace Umbraco.Infrastructure.PublishedCache.Persistence /// public bool VerifyContentDbCache() - => _repository.VerifyContentDbCache(); + { + using IScope scope = ScopeProvider.CreateScope(autoComplete: true); + scope.ReadLock(Constants.Locks.ContentTree); + return _repository.VerifyContentDbCache(); + } /// public bool VerifyMediaDbCache() - => _repository.VerifyMediaDbCache(); + { + using IScope scope = ScopeProvider.CreateScope(autoComplete: true); + scope.ReadLock(Constants.Locks.MediaTree); + return _repository.VerifyMediaDbCache(); + } /// public bool VerifyMemberDbCache() - => _repository.VerifyMemberDbCache(); + { + using IScope scope = ScopeProvider.CreateScope(autoComplete: true); + scope.ReadLock(Constants.Locks.MemberTree); + return _repository.VerifyMemberDbCache(); + } } } diff --git a/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotService.cs b/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotService.cs index e695517553..1ddb1cef63 100644 --- a/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotService.cs +++ b/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotService.cs @@ -4,6 +4,7 @@ using System.Globalization; using System.IO; using System.Linq; using System.Threading; +using System.Threading.Tasks; using CSharpTest.Net.Collections; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -47,7 +48,6 @@ namespace Umbraco.Web.PublishedCache.NuCache private readonly IIOHelper _ioHelper; private readonly NuCacheSettings _config; - // volatile because we read it with no lock private bool _isReady; private bool _isReadSet; private object _isReadyLock; @@ -264,7 +264,7 @@ namespace Umbraco.Web.PublishedCache.NuCache /// /// Populates the stores /// - private void EnsureCaches() => LazyInitializer.EnsureInitialized( + internal void EnsureCaches() => LazyInitializer.EnsureInitialized( ref _isReady, ref _isReadSet, ref _isReadyLock, @@ -914,7 +914,6 @@ namespace Umbraco.Web.PublishedCache.NuCache } // some may be missing - not checking here - return contentTypes.Select(x => _publishedContentTypeFactory.CreateContentType(x)).ToList(); } @@ -950,7 +949,6 @@ namespace Umbraco.Web.PublishedCache.NuCache // content (and content types) are read-locked while reading content // contentStore is wlocked (so readable, only no new views) // and it can be wlocked by 1 thread only at a time - using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.ContentTypes); @@ -989,7 +987,6 @@ namespace Umbraco.Web.PublishedCache.NuCache // media (and content types) are read-locked while reading media // mediaStore is wlocked (so readable, only no new views) // and it can be wlocked by 1 thread only at a time - using (var scope = _scopeProvider.CreateScope()) { scope.ReadLock(Constants.Locks.MediaTypes); @@ -1047,7 +1044,6 @@ namespace Umbraco.Web.PublishedCache.NuCache // nothing like that... // for elements cache, DictionaryAppCache is a No-No, use something better. // ie FastDictionaryAppCache (thread safe and all) - ContentStore.Snapshot contentSnap, mediaSnap; SnapDictionary.Snapshot domainSnap; IAppCache elementsCache; @@ -1059,7 +1055,7 @@ namespace Umbraco.Web.PublishedCache.NuCache lock (_elementsLock) { - var scopeContext = _scopeProvider.Context; + IScopeContext scopeContext = _scopeProvider.Context; if (scopeContext == null) { @@ -1089,7 +1085,6 @@ namespace Umbraco.Web.PublishedCache.NuCache }, int.MaxValue); } - // create a new snapshot cache if snapshots are different gens if (contentSnap.Gen != _contentGen || mediaSnap.Gen != _mediaGen || domainSnap.Gen != _domainGen || _elementsCache == null) { @@ -1126,75 +1121,12 @@ namespace Umbraco.Web.PublishedCache.NuCache IReadOnlyCollection memberTypeIds = null) => _publishedContentService.Rebuild(groupSize, contentTypeIds, mediaTypeIds, memberTypeIds); - public bool VerifyContentDbCache() - { - // TODO: Shouldn't this entire logic just exist in the call to _publishedContentService? - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.ContentTree); - var ok = _publishedContentService.VerifyContentDbCache(); - scope.Complete(); - return ok; - } - } - - public bool VerifyMediaDbCache() - { - // TODO: Shouldn't this entire logic just exist in the call to _publishedContentService? - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.MediaTree); - var ok = _publishedContentService.VerifyMediaDbCache(); - scope.Complete(); - return ok; - } - } - - public bool VerifyMemberDbCache() - { - // TODO: Shouldn't this entire logic just exist in the call to _publishedContentService? - using (var scope = _scopeProvider.CreateScope()) - { - scope.ReadLock(Constants.Locks.MemberTree); - var ok = _publishedContentService.VerifyMemberDbCache(); - scope.Complete(); - return ok; - } - } - - public string GetStatus() + public async Task CollectAsync() { EnsureCaches(); - var dbCacheIsOk = VerifyContentDbCache() - && VerifyMediaDbCache() - && VerifyMemberDbCache(); - - var cg = _contentStore.GenCount; - var mg = _mediaStore.GenCount; - var cs = _contentStore.SnapCount; - var ms = _mediaStore.SnapCount; - var ce = _contentStore.Count; - var me = _mediaStore.Count; - - return - " Database cache is " + (dbCacheIsOk ? "ok" : "NOT ok (rebuild?)") + "." + - " ContentStore contains " + ce + " item" + (ce > 1 ? "s" : "") + - " and has " + cg + " generation" + (cg > 1 ? "s" : "") + - " and " + cs + " snapshot" + (cs > 1 ? "s" : "") + "." + - " MediaStore contains " + me + " item" + (ce > 1 ? "s" : "") + - " and has " + mg + " generation" + (mg > 1 ? "s" : "") + - " and " + ms + " snapshot" + (ms > 1 ? "s" : "") + "."; - } - - // TODO: This should be async since it's calling into async - public void Collect() - { - EnsureCaches(); - - var contentCollect = _contentStore.CollectAsync(); - var mediaCollect = _mediaStore.CollectAsync(); - System.Threading.Tasks.Task.WaitAll(contentCollect, mediaCollect); + await _contentStore.CollectAsync(); + await _mediaStore.CollectAsync(); } internal ContentStore GetContentStore() @@ -1209,9 +1141,6 @@ namespace Umbraco.Web.PublishedCache.NuCache return _mediaStore; } - /// - public virtual string StatusUrl => "views/dashboard/settings/publishedsnapshotcache.html"; - /// public void Dispose() { } diff --git a/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotStatus.cs b/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotStatus.cs new file mode 100644 index 0000000000..c8975844ef --- /dev/null +++ b/src/Umbraco.PublishedCache.NuCache/PublishedSnapshotStatus.cs @@ -0,0 +1,51 @@ +using Umbraco.Infrastructure.PublishedCache.Persistence; + +namespace Umbraco.Web.PublishedCache.NuCache +{ + /// + /// Generates a status report for + /// + internal class PublishedSnapshotStatus : IPublishedSnapshotStatus + { + private readonly PublishedSnapshotService _service; + private readonly INuCacheContentService _publishedContentService; + + public PublishedSnapshotStatus(PublishedSnapshotService service, INuCacheContentService publishedContentService) + { + _service = service; + _publishedContentService = publishedContentService; + } + + /// + public virtual string StatusUrl => "views/dashboard/settings/publishedsnapshotcache.html"; + + /// + public string GetStatus() + { + _service.EnsureCaches(); + + var dbCacheIsOk = _publishedContentService.VerifyContentDbCache() + && _publishedContentService.VerifyMediaDbCache() + && _publishedContentService.VerifyMemberDbCache(); + + ContentStore contentStore = _service.GetContentStore(); + ContentStore mediaStore = _service.GetMediaStore(); + + var cg = contentStore.GenCount; + var mg = mediaStore.GenCount; + var cs = contentStore.SnapCount; + var ms = mediaStore.SnapCount; + var ce = contentStore.Count; + var me = mediaStore.Count; + + return + " Database cache is " + (dbCacheIsOk ? "ok" : "NOT ok (rebuild?)") + "." + + " ContentStore contains " + ce + " item" + (ce > 1 ? "s" : "") + + " and has " + cg + " generation" + (cg > 1 ? "s" : "") + + " and " + cs + " snapshot" + (cs > 1 ? "s" : "") + "." + + " MediaStore contains " + me + " item" + (ce > 1 ? "s" : "") + + " and has " + mg + " generation" + (mg > 1 ? "s" : "") + + " and " + ms + " snapshot" + (ms > 1 ? "s" : "") + "."; + } + } +} diff --git a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlPublishedSnapshotService.cs b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlPublishedSnapshotService.cs index 6b8b13cc99..0111c9f088 100644 --- a/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlPublishedSnapshotService.cs +++ b/src/Umbraco.Tests/LegacyXmlPublishedCache/XmlPublishedSnapshotService.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Umbraco.Core; using Umbraco.Core.Cache; @@ -177,8 +178,6 @@ namespace Umbraco.Tests.LegacyXmlPublishedCache #endregion - public string StatusUrl => throw new System.NotImplementedException(); - #region Xml specific /// @@ -258,13 +257,8 @@ namespace Umbraco.Tests.LegacyXmlPublishedCache #endregion - public string GetStatus() - { - return "Test status"; - } - public void Rebuild(int groupSize = 5000, IReadOnlyCollection contentTypeIds = null, IReadOnlyCollection mediaTypeIds = null, IReadOnlyCollection memberTypeIds = null) { } - public void Collect() { } + public Task CollectAsync() => Task.CompletedTask; } } diff --git a/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs b/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs index c9c420f254..90db13227f 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/PublishedSnapshotCacheStatusController.cs @@ -1,5 +1,6 @@ -using System; +using System; using System.Reflection.Metadata; +using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc; using Umbraco.Core; using Umbraco.Web.Cache; @@ -8,43 +9,54 @@ using Umbraco.Web.PublishedCache; namespace Umbraco.Web.BackOffice.Controllers { - [PluginController(Constants.Web.Mvc.BackOfficeApiArea)] + [PluginController(Constants.Web.Mvc.BackOfficeApiArea)] public class PublishedSnapshotCacheStatusController : UmbracoAuthorizedApiController { private readonly IPublishedSnapshotService _publishedSnapshotService; + private readonly IPublishedSnapshotStatus _publishedSnapshotStatus; private readonly DistributedCache _distributedCache; - public PublishedSnapshotCacheStatusController(IPublishedSnapshotService publishedSnapshotService, DistributedCache distributedCache) + /// + /// Initializes a new instance of the class. + /// + public PublishedSnapshotCacheStatusController( + IPublishedSnapshotService publishedSnapshotService, + IPublishedSnapshotStatus publishedSnapshotStatus, + DistributedCache distributedCache) { _publishedSnapshotService = publishedSnapshotService ?? throw new ArgumentNullException(nameof(publishedSnapshotService)); + _publishedSnapshotStatus = publishedSnapshotStatus; _distributedCache = distributedCache; } + /// + /// Rebuilds the Database cache + /// [HttpPost] public string RebuildDbCache() { _publishedSnapshotService.Rebuild(); - return _publishedSnapshotService.GetStatus(); + return _publishedSnapshotStatus.GetStatus(); } + /// + /// Gets a status report + /// [HttpGet] - public string GetStatus() - { - return _publishedSnapshotService.GetStatus(); - } + public string GetStatus() => _publishedSnapshotStatus.GetStatus(); + /// + /// Cleans up unused snapshots + /// [HttpGet] - public string Collect() + public async Task Collect() { GC.Collect(); - _publishedSnapshotService.Collect(); - return _publishedSnapshotService.GetStatus(); + await _publishedSnapshotService.CollectAsync(); + return _publishedSnapshotStatus.GetStatus(); } [HttpPost] - public void ReloadCache() - { - _distributedCache.RefreshAllPublishedSnapshot(); - } + public void ReloadCache() => _distributedCache.RefreshAllPublishedSnapshot(); } } diff --git a/src/Umbraco.Web.BackOffice/Controllers/PublishedStatusController.cs b/src/Umbraco.Web.BackOffice/Controllers/PublishedStatusController.cs index f63c2d5e6a..5c41d54cb8 100644 --- a/src/Umbraco.Web.BackOffice/Controllers/PublishedStatusController.cs +++ b/src/Umbraco.Web.BackOffice/Controllers/PublishedStatusController.cs @@ -1,4 +1,4 @@ -using System; +using System; using Microsoft.AspNetCore.Mvc; using Umbraco.Web.PublishedCache; @@ -6,22 +6,22 @@ namespace Umbraco.Web.BackOffice.Controllers { public class PublishedStatusController : UmbracoAuthorizedApiController { - private readonly IPublishedSnapshotService _publishedSnapshotService; + private readonly IPublishedSnapshotStatus _publishedSnapshotStatus; - public PublishedStatusController(IPublishedSnapshotService publishedSnapshotService) + public PublishedStatusController(IPublishedSnapshotStatus publishedSnapshotStatus) { - _publishedSnapshotService = publishedSnapshotService ?? throw new ArgumentNullException(nameof(publishedSnapshotService)); + _publishedSnapshotStatus = publishedSnapshotStatus ?? throw new ArgumentNullException(nameof(publishedSnapshotStatus)); } [HttpGet] public string GetPublishedStatusUrl() { - if (!string.IsNullOrWhiteSpace(_publishedSnapshotService.StatusUrl)) + if (!string.IsNullOrWhiteSpace(_publishedSnapshotStatus.StatusUrl)) { - return _publishedSnapshotService.StatusUrl; + return _publishedSnapshotStatus.StatusUrl; } - throw new NotSupportedException("Not supported: " + _publishedSnapshotService.GetType().FullName); + throw new NotSupportedException("Not supported: " + _publishedSnapshotStatus.GetType().FullName); } } }