yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Hybrid Cache: Resolve start-up errors with mis-matched types (#20554)

Browse Source 
* Be consistent in use of GetOrCreateAsync overload in exists and retrieval.
Ensure nullability of ContentCacheNode is consistent in exists and retrieval.

* Applied suggestion from code review.

* Move seeding to Umbraco application starting rather than started, ensuring an initial request is served.

* Tighten up hybrid cache exists check with locking around check and remove, and use of cancellation token.

(cherry picked from commit 81a8a0c191)
This commit is contained in:
Andy Butland
2025-10-21 09:57:29 +02:00
committed by mole
parent e6f48799a1
commit 68d1b9481a
7 changed files with 141 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
src/Umbraco.PublishedCache.HybridCache/DependencyInjection/UmbracoBuilderExtensions.cs
View File

@@ -1,4 +1,3 @@

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
@@ -74,7 +73,7 @@ public static class UmbracoBuilderExtensions
builder.AddNotificationAsyncHandler<ContentTypeDeletedNotification, CacheRefreshingNotificationHandler>();
builder.AddNotificationAsyncHandler<MediaTypeRefreshedNotification, CacheRefreshingNotificationHandler>();
builder.AddNotificationAsyncHandler<MediaTypeDeletedNotification, CacheRefreshingNotificationHandler>();
builder.AddNotificationAsyncHandler<UmbracoApplicationStartedNotification, SeedingNotificationHandler>();
builder.AddNotificationAsyncHandler<UmbracoApplicationStartingNotification, SeedingNotificationHandler>();
builder.AddCacheSeeding();
return builder;
}

67
src/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensions.cs
View File

@@ -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;
/// </summary>
internal static class HybridCacheExtensions
{
// Per-key semaphores to ensure the GetOrCreateAsync + RemoveAsync sequence
// executes atomically for a given cache key.
private static readonly ConcurrentDictionary<string, SemaphoreSlim> _keyLocks = new();
/// <summary>
/// Returns true if the cache contains an item with a matching key.
/// </summary>
/// <param name="cache">An instance of <see cref="Microsoft.Extensions.Caching.Hybrid.HybridCache"/></param>
/// <param name="key">The name (key) of the item to search for in the cache.</param>
/// <param name="token">The cancellation token.</param>
/// <returns>True if the item exists already. False if it doesn't.</returns>
/// <remarks>
/// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191
/// Will never add or alter the state of any items in the cache.
/// </remarks>
public static async Task<bool> ExistsAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key)
public static async Task<bool> ExistsAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token)
{
(bool exists, _) = await TryGetValueAsync<T>(cache, key);
(bool exists, _) = await TryGetValueAsync<T>(cache, key, token).ConfigureAwait(false);
return exists;
}
@@ -29,34 +35,55 @@ internal static class HybridCacheExtensions
/// <typeparam name="T">The type of the value of the item in the cache.</typeparam>
/// <param name="cache">An instance of <see cref="Microsoft.Extensions.Caching.Hybrid.HybridCache"/></param>
/// <param name="key">The name (key) of the item to search for in the cache.</param>
/// <param name="token">The cancellation token.</param>
/// <returns>A tuple of <see cref="bool"/> and the object (if found) retrieved from the cache.</returns>
/// <remarks>
/// Hat-tip: https://github.com/dotnet/aspnetcore/discussions/57191
/// Will never add or alter the state of any items in the cache.
/// </remarks>
public static async Task<(bool Exists, T? Value)> TryGetValueAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key)
public static async Task<(bool Exists, T? Value)> TryGetValueAsync<T>(this Microsoft.Extensions.Caching.Hybrid.HybridCache cache, string key, CancellationToken token)
{
var exists = true;
T? result = await cache.GetOrCreateAsync<object, T>(
key,
null!,
(_, _) =>
{
exists = false;
return new ValueTask<T>(default(T)!);
},
new HybridCacheEntryOptions(),
null,
CancellationToken.None);
// 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));
// 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)
await sem.WaitAsync().ConfigureAwait(false);
try
{
await cache.RemoveAsync(key);
}
T? result = await cache.GetOrCreateAsync<T?>(
key,
cancellationToken =>
{
exists = false;
return default;
},
new HybridCacheEntryOptions(),
null,
token).ConfigureAwait(false);
return (exists, result);
// 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();
// 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 _);
}
}
}
}

6
src/Umbraco.PublishedCache.HybridCache/NotificationHandlers/SeedingNotificationHandler.cs
View File

@@ -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 @@ using Umbraco.Cms.Infrastructure.HybridCache.Services;
namespace Umbraco.Cms.Infrastructure.HybridCache.NotificationHandlers;
internal sealed class SeedingNotificationHandler : INotificationAsyncHandler<UmbracoApplicationStartedNotification>
internal sealed class SeedingNotificationHandler : INotificationAsyncHandler<UmbracoApplicationStartingNotification>
{
private readonly IDocumentCacheService _documentCacheService;
private readonly IMediaCacheService _mediaCacheService;
@@ -24,7 +24,7 @@ internal sealed class SeedingNotificationHandler : INotificationAsyncHandler<Umb
_globalSettings = globalSettings.Value;
}
public async Task HandleAsync(UmbracoApplicationStartedNotification notification,
public async Task HandleAsync(UmbracoApplicationStartingNotification notification,
CancellationToken cancellationToken)
{

4
src/Umbraco.PublishedCache.HybridCache/Services/DocumentCacheService.cs
View File

@@ -205,7 +205,7 @@ internal sealed class DocumentCacheService : IDocumentCacheService
var cacheKey = GetCacheKey(key, false);
var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode>(cacheKey);
var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode?>(cacheKey, cancellationToken).ConfigureAwait(false);
if (existsInCache is false)
{
uncachedKeys.Add(key);
@@ -278,7 +278,7 @@ internal sealed class DocumentCacheService : IDocumentCacheService
return false;
}
return await _hybridCache.ExistsAsync<ContentCacheNode>(GetCacheKey(keyAttempt.Result, preview));
return await _hybridCache.ExistsAsync<ContentCacheNode?>(GetCacheKey(keyAttempt.Result, preview), CancellationToken.None);
}
public async Task RefreshContentAsync(IContent content)

4
src/Umbraco.PublishedCache.HybridCache/Services/MediaCacheService.cs
View File

@@ -133,7 +133,7 @@ internal sealed class MediaCacheService : IMediaCacheService
return false;
}
return await _hybridCache.ExistsAsync<ContentCacheNode>($"{keyAttempt.Result}");
return await _hybridCache.ExistsAsync<ContentCacheNode?>($"{keyAttempt.Result}", CancellationToken.None);
}
public async Task RefreshMediaAsync(IMedia media)
@@ -170,7 +170,7 @@ internal sealed class MediaCacheService : IMediaCacheService
var cacheKey = GetCacheKey(key, false);
var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode>(cacheKey);
var existsInCache = await _hybridCache.ExistsAsync<ContentCacheNode?>(cacheKey, CancellationToken.None);
if (existsInCache is false)
{
uncachedKeys.Add(key);

60
tests/Umbraco.Tests.Integration/Umbraco.PublishedCache.HybridCache/DocumentHybridCacheTests.cs
View File

@@ -8,6 +8,7 @@ using Umbraco.Cms.Core.Notifications;
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;
@@ -26,10 +27,10 @@ internal sealed class DocumentHybridCacheTests : UmbracoIntegrationTestWithConte
private IPublishedContentCache PublishedContentHybridCache => GetRequiredService<IPublishedContentCache>();
private IContentEditingService ContentEditingService => GetRequiredService<IContentEditingService>();
private IContentPublishingService ContentPublishingService => GetRequiredService<IContentPublishingService>();
private IDocumentCacheService DocumentCacheService => GetRequiredService<IDocumentCacheService>();
private const string NewName = "New Name";
private const string NewTitle = "New Title";
@@ -467,6 +468,61 @@ internal sealed class DocumentHybridCacheTests : UmbracoIntegrationTestWithConte
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(() =>

55
tests/Umbraco.Tests.UnitTests/Umbraco.PublishedCache.HybridCache/Extensions/HybridCacheExtensionsTests.cs
View File

@@ -1,3 +1,4 @@
using System;
using Microsoft.Extensions.Caching.Hybrid;
using Moq;
using NUnit.Framework;
@@ -33,15 +34,15 @@ public class HybridCacheExtensionsTests
_cacheMock
.Setup(cache => cache.GetOrCreateAsync(
key,
null!,
It.IsAny<Func<object, CancellationToken, ValueTask<ContentCacheNode>>>(),
It.IsAny<Func<CancellationToken, ValueTask<ContentCacheNode?>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<ContentCacheNode?>>, CancellationToken, ValueTask<ContentCacheNode?>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.ReturnsAsync(expectedValue);
// Act
var exists = await HybridCacheExtensions.ExistsAsync<ContentCacheNode>(_cacheMock.Object, key);
var exists = await HybridCacheExtensions.ExistsAsync<ContentCacheNode?>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsTrue(exists);
@@ -56,24 +57,24 @@ public class HybridCacheExtensionsTests
_cacheMock
.Setup(cache => cache.GetOrCreateAsync(
key,
null!,
It.IsAny<Func<object, CancellationToken, ValueTask<ContentCacheNode>>>(),
It.IsAny<Func<CancellationToken, ValueTask<ContentCacheNode?>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<ContentCacheNode?>>, CancellationToken, ValueTask<ContentCacheNode?>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.Returns((
string key,
object? state,
Func<object, CancellationToken, ValueTask<ContentCacheNode>> factory,
Func<CancellationToken, ValueTask<ContentCacheNode?>> state,
Func<Func<CancellationToken, ValueTask<ContentCacheNode?>>, CancellationToken, ValueTask<ContentCacheNode?>> factory,
HybridCacheEntryOptions? options,
IEnumerable<string>? tags,
CancellationToken token) =>
{
return factory(state!, token);
return factory(state, token);
});
// Act
var exists = await HybridCacheExtensions.ExistsAsync<ContentCacheNode>(_cacheMock.Object, key);
var exists = await HybridCacheExtensions.ExistsAsync<ContentCacheNode?>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsFalse(exists);
@@ -89,15 +90,15 @@ public class HybridCacheExtensionsTests
_cacheMock
.Setup(cache => cache.GetOrCreateAsync(
key,
null!,
It.IsAny<Func<object, CancellationToken, ValueTask<string>>>(),
It.IsAny<Func<CancellationToken, ValueTask<string>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<string>>, CancellationToken, ValueTask<string>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.ReturnsAsync(expectedValue);
// Act
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<string>(_cacheMock.Object, key);
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<string>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsTrue(exists);
@@ -114,15 +115,15 @@ public class HybridCacheExtensionsTests
_cacheMock
.Setup(cache => cache.GetOrCreateAsync(
key,
null!,
It.IsAny<Func<object, CancellationToken, ValueTask<int>>>(),
It.IsAny<Func<CancellationToken, ValueTask<int>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<int>>, CancellationToken, ValueTask<int>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.ReturnsAsync(expectedValue);
// Act
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<int>(_cacheMock.Object, key);
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<int>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsTrue(exists);
@@ -138,15 +139,15 @@ public class HybridCacheExtensionsTests
_cacheMock
.Setup(cache => cache.GetOrCreateAsync(
key,
null!,
It.IsAny<Func<object, CancellationToken, ValueTask<object>>>(),
It.IsAny<Func<CancellationToken, ValueTask<object>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<object>>, CancellationToken, ValueTask<object>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.ReturnsAsync(null!);
// Act
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<int?>(_cacheMock.Object, key);
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<int?>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsTrue(exists);
@@ -160,16 +161,16 @@ public class HybridCacheExtensionsTests
string key = "test-key";
_cacheMock.Setup(cache => cache.GetOrCreateAsync(
key,
null,
It.IsAny<Func<object?, CancellationToken, ValueTask<string>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
key,
It.IsAny<Func<CancellationToken, ValueTask<object>>>(),
It.IsAny<Func<Func<CancellationToken, ValueTask<object>>, CancellationToken, ValueTask<object>>>(),
It.IsAny<HybridCacheEntryOptions>(),
null,
CancellationToken.None))
.Returns((
string key,
object? state,
Func<object?, CancellationToken, ValueTask<string>> factory,
Func<CancellationToken, ValueTask<object>> state,
Func<Func<CancellationToken, ValueTask<object>>, CancellationToken, ValueTask<object>> factory,
HybridCacheEntryOptions? options,
IEnumerable<string>? tags,
CancellationToken token) =>
@@ -178,7 +179,7 @@ public class HybridCacheExtensionsTests
});
// Act
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<string>(_cacheMock.Object, key);
var (exists, value) = await HybridCacheExtensions.TryGetValueAsync<object>(_cacheMock.Object, key, CancellationToken.None);
// Assert
Assert.IsFalse(exists);