From fea86a1bee6267aa5c3908ec413365994b5b1d2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Luthi?= Date: Tue, 28 May 2024 14:13:58 +0200 Subject: [PATCH 1/5] Use GetRequiredService instead of GetService and null-forgiving operator An `InvalidOperationException` is always better than a `NullReferenceException`, even in unit tests. --- .../DependencyInjectionTests.cs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs index 1cd908fc..6c75b881 100644 --- a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs +++ b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs @@ -508,7 +508,7 @@ public void CanUseMultipleNamedCachesAndConfigureThem() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var fooCache = cacheProvider.GetCache("FooCache"); var barCache = cacheProvider.GetCache("BarCache"); @@ -574,7 +574,7 @@ public void CanUseDefaultCacheWithMultipleNamedCaches() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var fooCache = cacheProvider.GetCache("FooCache"); var barCache = cacheProvider.GetCache("BarCache"); @@ -613,7 +613,7 @@ public void CanUsePostSetupActions() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var cache = cacheProvider.GetDefaultCache(); @@ -645,7 +645,7 @@ public void CanResetPostSetupActions() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var cache = cacheProvider.GetDefaultCache(); @@ -663,7 +663,7 @@ public void DontThrowWhenRequestingAnUnregisteredCache() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); Assert.Null(cacheProvider.GetCacheOrNull("BarCache")); } @@ -678,7 +678,7 @@ public void DefaultCacheIsTheSameWhenRequestedInDifferentWays() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); Assert.Equal(cacheProvider.GetDefaultCache(), serviceProvider.GetService()); } @@ -693,7 +693,7 @@ public void ThrowsOrNotWhenRequestingUnregisteredNamedCaches() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); Assert.Throws(() => { @@ -728,7 +728,7 @@ public void ThrowsOrNotWhenRequestingUnregisteredDefaultCache() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); Assert.Throws(() => { @@ -753,7 +753,7 @@ public void CacheInstancesAreAlwaysTheSame() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var defaultCache1 = cacheProvider.GetDefaultCache(); var defaultCache2 = cacheProvider.GetDefaultCache(); @@ -787,7 +787,7 @@ public void DifferentNamedCachesDoNotShareTheSameMemoryCacheByDefault() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var defaultCache = cacheProvider.GetDefaultCache(); var fooCache = cacheProvider.GetCache("FooCache"); @@ -826,7 +826,7 @@ public void DifferentNamedCachesCanShareTheSameMemoryCacheWithCollisions() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var defaultCache = cacheProvider.GetDefaultCache(); var fooCache = cacheProvider.GetCache("FooCache"); @@ -865,7 +865,7 @@ public void DifferentNamedCachesCanShareTheSameMemoryCacheWithoutCollisions() using var serviceProvider = services.BuildServiceProvider(); - var cacheProvider = serviceProvider.GetService()!; + var cacheProvider = serviceProvider.GetRequiredService(); var defaultCache = cacheProvider.GetDefaultCache(); var fooCache = cacheProvider.GetCache("FooCache"); From c43c4e6a8fb79c6f5936b8e461d78b5063d9deda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Luthi?= Date: Tue, 28 May 2024 11:08:53 +0200 Subject: [PATCH 2/5] Ensure that the default IFusionCache instance is always the same No matter how it's retrieved, either through IFusionCacheProvider or directly by requesting the IFusionCache. --- .../DependencyInjectionTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs index 6c75b881..258e6bad 100644 --- a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs +++ b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs @@ -757,6 +757,7 @@ public void CacheInstancesAreAlwaysTheSame() var defaultCache1 = cacheProvider.GetDefaultCache(); var defaultCache2 = cacheProvider.GetDefaultCache(); + var defaultCache3 = serviceProvider.GetRequiredService(); var fooCache1 = cacheProvider.GetCache("Foo"); var fooCache2 = cacheProvider.GetCache("Foo"); @@ -765,6 +766,7 @@ public void CacheInstancesAreAlwaysTheSame() var barCache2 = cacheProvider.GetCache("Bar"); Assert.Same(defaultCache1, defaultCache2); + Assert.Same(defaultCache2, defaultCache3); Assert.Same(fooCache1, fooCache2); Assert.Same(barCache1, barCache2); } From d818554b4e64fc6bf3acd5fe3ff97a25922cfc59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Luthi?= Date: Tue, 28 May 2024 11:11:24 +0200 Subject: [PATCH 3/5] Improve the performance of the FusionCacheProvider By storing the caches in a dictionary so that retrieving the cache can be performed in O(1) instead of O(n). --- .../Internals/Provider/FusionCacheProvider.cs | 29 +++++++++---------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs b/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs index 7b54922f..baf73a1b 100644 --- a/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs +++ b/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs @@ -7,29 +7,26 @@ namespace ZiggyCreatures.Caching.Fusion.Internals.Provider; internal sealed class FusionCacheProvider : IFusionCacheProvider { - private readonly IFusionCache? _defaultCache; - private readonly LazyNamedCache[] _lazyNamedCaches; + private readonly Dictionary> _caches; public FusionCacheProvider(IEnumerable defaultCaches, IEnumerable lazyNamedCaches) { - _defaultCache = defaultCaches.LastOrDefault(); - _lazyNamedCaches = lazyNamedCaches.ToArray(); + _caches = new Dictionary>(); + foreach (var group in lazyNamedCaches.GroupBy(g => g.CacheName)) + { + _caches.Add(group.Key, new Lazy(() => group.Count() == 1 ? group.First().Cache : throw new InvalidOperationException($"Multiple FusionCache registrations have been found with the provided name ({group.Key})"))); + } + + var defaultCache = defaultCaches.LastOrDefault(); + if (defaultCache != null) + { + _caches.Add(FusionCacheOptions.DefaultCacheName, new Lazy(() => defaultCache)); + } } public IFusionCache? GetCacheOrNull(string cacheName) { - if (cacheName == FusionCacheOptions.DefaultCacheName) - return _defaultCache; - - var matchingLazyNamedCaches = _lazyNamedCaches.Where(x => x.CacheName == cacheName).ToArray(); - - if (matchingLazyNamedCaches.Length == 1) - return matchingLazyNamedCaches[0].Cache; - - if (matchingLazyNamedCaches.Length > 1) - throw new InvalidOperationException($"Multiple FusionCache registrations have been found with the provided name ({cacheName})"); - - return null; + return _caches.TryGetValue(cacheName, out var cache) ? cache.Value : null; } public IFusionCache GetCache(string cacheName) From e0bc372affb95a9ef65221714dd240877b0cd53d Mon Sep 17 00:00:00 2001 From: Jody Donetti Date: Tue, 4 Jun 2024 14:39:57 +0200 Subject: [PATCH 4/5] FusionCacheProvider optimization --- .../Internals/Provider/FusionCacheProvider.cs | 29 +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs b/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs index baf73a1b..ca07492d 100644 --- a/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs +++ b/src/ZiggyCreatures.FusionCache/Internals/Provider/FusionCacheProvider.cs @@ -7,26 +7,43 @@ namespace ZiggyCreatures.Caching.Fusion.Internals.Provider; internal sealed class FusionCacheProvider : IFusionCacheProvider { - private readonly Dictionary> _caches; + private readonly Dictionary _caches; public FusionCacheProvider(IEnumerable defaultCaches, IEnumerable lazyNamedCaches) { - _caches = new Dictionary>(); + _caches = []; foreach (var group in lazyNamedCaches.GroupBy(g => g.CacheName)) { - _caches.Add(group.Key, new Lazy(() => group.Count() == 1 ? group.First().Cache : throw new InvalidOperationException($"Multiple FusionCache registrations have been found with the provided name ({group.Key})"))); + if (group.Count() == 1) + { + // ONLY 1 CACHE -> ADD IT + _caches[group.Key] = group.First(); + } + else + { + // MORE THAN 1 CACHE -> ADD NULL + // NOTE: THIS WILL SIGNAL THAT THERE WERE MULTIPLE ONES AND, SINCE + // THEY WILL NOT BE ACCESSIBLE ANYWAY, WILL SAVE SOME MEMORY + _caches[group.Key] = null; + } } var defaultCache = defaultCaches.LastOrDefault(); - if (defaultCache != null) + if (defaultCache is not null) { - _caches.Add(FusionCacheOptions.DefaultCacheName, new Lazy(() => defaultCache)); + _caches[defaultCache.CacheName] = new LazyNamedCache(defaultCache.CacheName, defaultCache); } } public IFusionCache? GetCacheOrNull(string cacheName) { - return _caches.TryGetValue(cacheName, out var cache) ? cache.Value : null; + if (_caches.TryGetValue(cacheName, out var item) == false) + return null; + + if (item is null) + throw new InvalidOperationException($"Multiple FusionCache registrations have been found with the provided name ({cacheName})"); + + return item.Cache; } public IFusionCache GetCache(string cacheName) From 496bd25a629e93a037ab5213b851e2dca75837ba Mon Sep 17 00:00:00 2001 From: Jody Donetti Date: Tue, 4 Jun 2024 14:40:08 +0200 Subject: [PATCH 5/5] Better FUsionCacheProvider tests --- .../DependencyInjectionTests.cs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs index 40065801..5d1881be 100644 --- a/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs +++ b/tests/ZiggyCreatures.FusionCache.Tests/DependencyInjectionTests.cs @@ -446,6 +446,33 @@ public void ThrowsIfMissingSerializerWhenUsingDistributedCache() }); } + [Fact] + public void CanRegisterMultipleDefaultCaches() + { + // NOTE: EVEN THOUGH IT'S POSSIBLE TO REGISTER MULTIPLE DEFAULT CACHES, IT'S NOT RECOMMENDED, + // AND IT'S NOT POSSIBLE TO USE THEM IN A MEANINGFUL WAY, AS THE LAST ONE REGISTERED WILL BE THE ONE USED. + // THIS FOLLOWS THE STANDARD BEHAVIOR OF MICROSOFT'S DI CONTAINER. + + var services = new ServiceCollection(); + + services.AddFusionCache(); + services.AddFusionCache(); + + using var serviceProvider = services.BuildServiceProvider(); + + var cacheProvider = serviceProvider.GetRequiredService(); + + var cache1 = cacheProvider.GetDefaultCache(); + var cache2 = cacheProvider.GetDefaultCache(); + var cache3 = serviceProvider.GetRequiredService(); + + Assert.NotNull(cache1); + Assert.NotNull(cache2); + Assert.NotNull(cache3); + Assert.Equal(cache1, cache2); + Assert.Equal(cache2, cache3); + } + [Fact] public void CanUseMultipleNamedCachesAndConfigureThem() { @@ -717,6 +744,15 @@ public void ThrowsOrNotWhenRequestingUnregisteredNamedCaches() var maybeBarCache = cacheProvider.GetCacheOrNull("Bar"); Assert.Null(maybeBarCache); + + Assert.Throws(() => + { + // NO DEFAULT CACHE REGISTERED -> THROWS + _ = cacheProvider.GetDefaultCache(); + }); + + // NO DEFAULT CACHE REGISTERED -> RETURNS NULL + var defaultCache = cacheProvider.GetDefaultCacheOrNull(); } [Fact]