diff --git a/src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs b/src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs index ca625dacdf5c..e97c60fd6253 100644 --- a/src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs +++ b/src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs @@ -1,4 +1,3 @@ - using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Umbraco.Cms.Core.Configuration.Models; @@ -74,7 +73,7 @@ public static IUmbracoBuilder AddUmbracoHybridCache(this IUmbracoBuilder builder builder.AddNotificationAsyncHandler(); builder.AddNotificationAsyncHandler(); builder.AddNotificationAsyncHandler(); - builder.AddNotificationAsyncHandler(); + builder.AddNotificationAsyncHandler(); builder.AddCacheSeeding(); return builder; } diff --git a/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs b/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs index ee1b7aefda05..ccd5897494eb 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using Microsoft.Extensions.Caching.Hybrid; namespace Umbraco.Cms.Infrastructure.HybridCache.Extensions; @@ -7,19 +8,24 @@ namespace Umbraco.Cms.Infrastructure.HybridCache.Extensions; /// internal static class HybridCacheExtensions { + // Per-key semaphores to ensure the GetOrCreateAsync + RemoveAsync sequence + // executes atomically for a given cache key. + private static readonly ConcurrentDictionary _keyLocks = new(); + /// /// Returns true if the cache contains an item with a matching key. /// /// An instance of /// The name (key) of the item to search for in the cache. + /// The cancellation token. /// True if the item exists already. False if it doesn't. /// /// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191 /// Will never add or alter the state of any items in the cache. /// - public static async Task ExistsAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key) + public static async Task ExistsAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token) { - (bool exists, _) = await TryGetValueAsync(cache, key); + (bool exists, _) = await TryGetValueAsync(cache, key, token).ConfigureAwait(false); return exists; } @@ -29,34 +35,55 @@ public static async Task ExistsAsync(this Microsoft.Extensions.Caching. /// The type of the value of the item in the cache. /// An instance of /// The name (key) of the item to search for in the cache. + /// The cancellation token. /// A tuple of and the object (if found) retrieved from the cache. /// /// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191 /// Will never add or alter the state of any items in the cache. /// - public static async Task<(bool Exists, T? Value)> TryGetValueAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key) + public static async Task<(bool Exists, T? Value)> TryGetValueAsync(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token) { var exists = true; - T? result = await cache.GetOrCreateAsync( - key, - null!, - (_, _) => - { - exists = false; - return new ValueTask(default(T)!); - }, - new HybridCacheEntryOptions(), - null, - CancellationToken.None); - - // In checking for the existence of the item, if not found, we will have created a cache entry with a null value. - // So remove it again. - if (exists is false) + // Acquire a per-key semaphore so that GetOrCreateAsync and the possible RemoveAsync + // complete without another thread retrieving/creating the same key in-between. + SemaphoreSlim sem = _keyLocks.GetOrAdd(key, _ => new SemaphoreSlim(1, 1)); + + await sem.WaitAsync().ConfigureAwait(false); + + try { - await cache.RemoveAsync(key); + T? result = await cache.GetOrCreateAsync( + key, + cancellationToken => + { + exists = false; + return default; + }, + new HybridCacheEntryOptions(), + null, + token).ConfigureAwait(false); + + // In checking for the existence of the item, if not found, we will have created a cache entry with a null value. + // So remove it again. Because we're holding the per-key lock there is no chance another thread + // will observe the temporary entry between GetOrCreateAsync and RemoveAsync. + if (exists is false) + { + await cache.RemoveAsync(key).ConfigureAwait(false); + } + + return (exists, result); } + finally + { + sem.Release(); - return (exists, result); + // Only remove the semaphore mapping if it still points to the same instance we used. + // This avoids removing another thread's semaphore or corrupting the map. + if (_keyLocks.TryGetValue(key, out SemaphoreSlim? current) && ReferenceEquals(current, sem)) + { + _keyLocks.TryRemove(key, out _); + } + } } } diff --git a/src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs b/src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs index 0581bd26542c..38a6618c7019 100644 --- a/src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs +++ b/src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs @@ -1,4 +1,4 @@ -using Microsoft.Extensions.Options; +using Microsoft.Extensions.Options; using Umbraco.Cms.Core; using Umbraco.Cms.Core.Configuration.Models; using Umbraco.Cms.Core.Events; @@ -9,7 +9,7 @@ namespace Umbraco.Cms.Infrastructure.HybridCache.NotificationHandlers; -internal sealed class SeedingNotificationHandler : INotificationAsyncHandler +internal sealed class SeedingNotificationHandler : INotificationAsyncHandler { private readonly IDocumentCacheService _documentCacheService; private readonly IMediaCacheService _mediaCacheService; @@ -24,7 +24,7 @@ public SeedingNotificationHandler(IDocumentCacheService documentCacheService, IM _globalSettings = globalSettings.Value; } - public async Task HandleAsync(UmbracoApplicationStartedNotification notification, + public async Task HandleAsync(UmbracoApplicationStartingNotification notification, CancellationToken cancellationToken) { diff --git a/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs b/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs index 1675cb05cfb2..2879be5c8d7f 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs @@ -205,7 +205,7 @@ public async Task SeedAsync(CancellationToken cancellationToken) var cacheKey = GetCacheKey(key, false); - var existsInCache = await _hybridCache.ExistsAsync(cacheKey); + var existsInCache = await _hybridCache.ExistsAsync(cacheKey, cancellationToken).ConfigureAwait(false); if (existsInCache is false) { uncachedKeys.Add(key); @@ -278,7 +278,7 @@ public async Task HasContentByIdAsync(int id, bool preview = false) return false; } - return await _hybridCache.ExistsAsync(GetCacheKey(keyAttempt.Result, preview)); + return await _hybridCache.ExistsAsync(GetCacheKey(keyAttempt.Result, preview), CancellationToken.None); } public async Task RefreshContentAsync(IContent content) diff --git a/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs b/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs index 65b8f91945a5..46d782bdbecb 100644 --- a/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs +++ b/src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs @@ -133,7 +133,7 @@ public async Task HasContentByIdAsync(int id) return false; } - return await _hybridCache.ExistsAsync($"{keyAttempt.Result}"); + return await _hybridCache.ExistsAsync($"{keyAttempt.Result}", CancellationToken.None); } public async Task RefreshMediaAsync(IMedia media) @@ -170,7 +170,7 @@ public async Task SeedAsync(CancellationToken cancellationToken) var cacheKey = GetCacheKey(key, false); - var existsInCache = await _hybridCache.ExistsAsync(cacheKey); + var existsInCache = await _hybridCache.ExistsAsync(cacheKey, CancellationToken.None); if (existsInCache is false) { uncachedKeys.Add(key); diff --git a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs index 0c4207331a0e..088f14cd319a 100644 --- a/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs +++ b/tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs @@ -7,6 +7,7 @@ using Umbraco.Cms.Core.PublishedCache; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Sync; +using Umbraco.Cms.Tests.Common.Builders; using Umbraco.Cms.Tests.Common.Testing; using Umbraco.Cms.Tests.Integration.Testing; using Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services; @@ -25,10 +26,10 @@ protected override void CustomTestSetup(IUmbracoBuilder builder) private IPublishedContentCache PublishedContentHybridCache => GetRequiredService(); - private IContentEditingService ContentEditingService => GetRequiredService(); - private IContentPublishingService ContentPublishingService => GetRequiredService(); + private IDocumentCacheService DocumentCacheService => GetRequiredService(); + private const string NewName = "New Name"; private const string NewTitle = "New Title"; @@ -460,6 +461,61 @@ public async Task Can_Not_Get_Deleted_Published_Content_By_Key(bool preview) Assert.IsNull(textPage); } + [Test] + public async Task Can_Get_Published_Content_By_Id_After_Previous_Check_Where_Not_Found() + { + // Arrange + var testPageKey = Guid.NewGuid(); + + // Act & Assert + // - assert we cannot get the content that doesn't yet exist from the cache + var testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey); + Assert.IsNull(testPage); + + testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey); + Assert.IsNull(testPage); + + // - create and publish the content + var testPageContent = ContentEditingBuilder.CreateBasicContent(ContentType.Key, testPageKey); + var createResult = await ContentEditingService.CreateAsync(testPageContent, Constants.Security.SuperUserKey); + Assert.IsTrue(createResult.Success); + var publishResult = await ContentPublishingService.PublishAsync(testPageKey, CultureAndSchedule, Constants.Security.SuperUserKey); + Assert.IsTrue(publishResult.Success); + + // - assert we can now get the content from the cache + testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey); + Assert.IsNotNull(testPage); + } + + [Test] + public async Task Can_Get_Published_Content_By_Id_After_Previous_Exists_Check() + { + // Act + var hasContentForTextPageCached = await DocumentCacheService.HasContentByIdAsync(PublishedTextPageId); + Assert.IsTrue(hasContentForTextPageCached); + var textPage = await PublishedContentHybridCache.GetByIdAsync(PublishedTextPageId); + + // Assert + AssertPublishedTextPage(textPage); + } + + [Test] + public async Task Can_Do_Exists_Check_On_Created_Published_Content() + { + var testPageKey = Guid.NewGuid(); + var testPageContent = ContentEditingBuilder.CreateBasicContent(ContentType.Key, testPageKey); + var createResult = await ContentEditingService.CreateAsync(testPageContent, Constants.Security.SuperUserKey); + Assert.IsTrue(createResult.Success); + var publishResult = await ContentPublishingService.PublishAsync(testPageKey, CultureAndSchedule, Constants.Security.SuperUserKey); + Assert.IsTrue(publishResult.Success); + + var testPage = await PublishedContentHybridCache.GetByIdAsync(testPageKey); + Assert.IsNotNull(testPage); + + var hasContentForTextPageCached = await DocumentCacheService.HasContentByIdAsync(testPage.Id); + Assert.IsTrue(hasContentForTextPageCached); + } + private void AssertTextPage(IPublishedContent textPage) { Assert.Multiple(() => diff --git a/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs b/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs index 152fe28b4ef6..8da30cd11883 100644 --- a/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs +++ b/tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs @@ -1,3 +1,4 @@ +using System; using Microsoft.Extensions.Caching.Hybrid; using Moq; using NUnit.Framework; @@ -33,15 +34,15 @@ public async Task ExistsAsync_WhenKeyExists_ShouldReturnTrue() _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, - null!, - It.IsAny>>(), + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), It.IsAny(), null, CancellationToken.None)) .ReturnsAsync(expectedValue); // Act - var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); + var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsTrue(exists); @@ -56,24 +57,24 @@ public async Task ExistsAsync_WhenKeyDoesNotExist_ShouldReturnFalse() _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, - null!, - It.IsAny>>(), + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), It.IsAny(), null, CancellationToken.None)) .Returns(( string key, - object? state, - Func> factory, + Func> state, + Func>, CancellationToken, ValueTask> factory, HybridCacheEntryOptions? options, IEnumerable? tags, CancellationToken token) => { - return factory(state!, token); + return factory(state, token); }); // Act - var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key); + var exists = await HybridCacheExtensions.ExistsAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsFalse(exists); @@ -89,15 +90,15 @@ public async Task TryGetValueAsync_WhenKeyExists_ShouldReturnTrueAndValueAsStrin _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, - null!, - It.IsAny>>(), + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), It.IsAny(), null, CancellationToken.None)) .ReturnsAsync(expectedValue); // Act - var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key); + var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsTrue(exists); @@ -114,15 +115,15 @@ public async Task TryGetValueAsync_WhenKeyExists_ShouldReturnTrueAndValueAsInteg _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, - null!, - It.IsAny>>(), + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), It.IsAny(), null, CancellationToken.None)) .ReturnsAsync(expectedValue); // Act - var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key); + var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsTrue(exists); @@ -138,15 +139,15 @@ public async Task TryGetValueAsync_WhenKeyExistsButValueIsNull_ShouldReturnTrueA _cacheMock .Setup(cache => cache.GetOrCreateAsync( key, - null!, - It.IsAny>>(), + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), It.IsAny(), null, CancellationToken.None)) .ReturnsAsync(null!); // Act - var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key); + var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsTrue(exists); @@ -160,16 +161,16 @@ public async Task TryGetValueAsync_WhenKeyDoesNotExist_ShouldReturnFalseAndNull( string key = "test-key"; _cacheMock.Setup(cache => cache.GetOrCreateAsync( - key, - null, - It.IsAny>>(), - It.IsAny(), - null, - CancellationToken.None)) + key, + It.IsAny>>(), + It.IsAny>, CancellationToken, ValueTask>>(), + It.IsAny(), + null, + CancellationToken.None)) .Returns(( string key, - object? state, - Func> factory, + Func> state, + Func>, CancellationToken, ValueTask> factory, HybridCacheEntryOptions? options, IEnumerable? tags, CancellationToken token) => @@ -178,7 +179,7 @@ public async Task TryGetValueAsync_WhenKeyDoesNotExist_ShouldReturnFalseAndNull( }); // Act - var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key); + var (exists, value) = await HybridCacheExtensions.TryGetValueAsync(_cacheMock.Object, key, CancellationToken.None); // Assert Assert.IsFalse(exists);