From df8d00101f5481553a7646f434fc970cdc0e9d1e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 3 Dec 2020 19:23:32 +0100 Subject: [PATCH 1/4] CacheEntryStack._previous is never used so it can be removed --- .../src/CacheEntryStack.cs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs index 9b6a9b71abf41c..198d50230c0d7f 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs @@ -1,27 +1,18 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; - namespace Microsoft.Extensions.Caching.Memory { internal class CacheEntryStack { - private readonly CacheEntryStack _previous; private readonly CacheEntry _entry; private CacheEntryStack() { } - private CacheEntryStack(CacheEntryStack previous, CacheEntry entry) + private CacheEntryStack(CacheEntry entry) { - if (previous == null) - { - throw new ArgumentNullException(nameof(previous)); - } - - _previous = previous; _entry = entry; } @@ -29,7 +20,7 @@ private CacheEntryStack(CacheEntryStack previous, CacheEntry entry) public CacheEntryStack Push(CacheEntry c) { - return new CacheEntryStack(this, c); + return new CacheEntryStack(c); } public CacheEntry Peek() From 8789cb5bfd3c2f4540d3344161f981172db57c5e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 3 Dec 2020 19:52:22 +0100 Subject: [PATCH 2/4] remove CacheEntryStack and ScopeLease as it's enough to have just AsyncLocal --- .../src/CacheEntry.cs | 16 +++--- .../src/CacheEntryHelper.cs | 55 ++++--------------- .../src/CacheEntryStack.cs | 31 ----------- 3 files changed, 18 insertions(+), 84 deletions(-) delete mode 100644 src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs index 47e3c5c8fd4177..6f91cb21f038cd 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs @@ -24,7 +24,7 @@ internal class CacheEntry : ICacheEntry private TimeSpan? _absoluteExpirationRelativeToNow; private TimeSpan? _slidingExpiration; private long? _size; - private IDisposable _scope; + private CacheEntry _previous; // this field is not null only before the entry is added to the cache private object _value; private int _state; // actually a [Flag] enum called "State" @@ -32,7 +32,7 @@ internal CacheEntry(object key, MemoryCache memoryCache) { Key = key ?? throw new ArgumentNullException(nameof(key)); _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache)); - _scope = CacheEntryHelper.EnterScope(this); + _previous = CacheEntryHelper.EnterScope(this); } /// @@ -145,11 +145,6 @@ public void Dispose() { IsDisposed = true; - // Ensure the _scope reference is cleared because it can reference other CacheEntry instances. - // This CacheEntry is going to be put into a MemoryCache, and we don't want to root unnecessary objects. - _scope.Dispose(); - _scope = null; - // Don't commit or propagate options if the CacheEntry Value was never set. // We assume an exception occurred causing the caller to not set the Value successfully, // so don't use this entry. @@ -157,11 +152,14 @@ public void Dispose() { _cache.SetEntry(this); - if (CanPropagateOptions()) + if (_previous != null && CanPropagateOptions()) { - PropagateOptions(CacheEntryHelper.Current); + PropagateOptions(_previous); } } + + CacheEntryHelper.ExitScope(this, _previous); + _previous = null; // we don't want to root unnecessary objects } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs index f7cc3a8d735e80..6877d08b7b1a64 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs @@ -1,65 +1,32 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; +using System.Diagnostics; using System.Threading; namespace Microsoft.Extensions.Caching.Memory { internal class CacheEntryHelper { - private static readonly AsyncLocal _scopes = new AsyncLocal(); - - internal static CacheEntryStack Scopes - { - get { return _scopes.Value; } - set { _scopes.Value = value; } - } + private static readonly AsyncLocal _current = new AsyncLocal(); internal static CacheEntry Current { - get - { - CacheEntryStack scopes = GetOrCreateScopes(); - return scopes.Peek(); - } + get => _current.Value; + private set => _current.Value = value; } - internal static IDisposable EnterScope(CacheEntry entry) + internal static CacheEntry EnterScope(CacheEntry current) { - CacheEntryStack scopes = GetOrCreateScopes(); - - var scopeLease = new ScopeLease(scopes); - Scopes = scopes.Push(entry); - - return scopeLease; + CacheEntry previous = Current; + Current = current; + return previous; } - private static CacheEntryStack GetOrCreateScopes() + internal static void ExitScope(CacheEntry current, CacheEntry previous) { - CacheEntryStack scopes = Scopes; - if (scopes == null) - { - scopes = CacheEntryStack.Empty; - Scopes = scopes; - } - - return scopes; - } - - private sealed class ScopeLease : IDisposable - { - private readonly CacheEntryStack _cacheEntryStack; - - public ScopeLease(CacheEntryStack cacheEntryStack) - { - _cacheEntryStack = cacheEntryStack; - } - - public void Dispose() - { - Scopes = _cacheEntryStack; - } + Debug.Assert(Current == current); + Current = previous; } } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs deleted file mode 100644 index 198d50230c0d7f..00000000000000 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs +++ /dev/null @@ -1,31 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -namespace Microsoft.Extensions.Caching.Memory -{ - internal class CacheEntryStack - { - private readonly CacheEntry _entry; - - private CacheEntryStack() - { - } - - private CacheEntryStack(CacheEntry entry) - { - _entry = entry; - } - - public static CacheEntryStack Empty { get; } = new CacheEntryStack(); - - public CacheEntryStack Push(CacheEntry c) - { - return new CacheEntryStack(c); - } - - public CacheEntry Peek() - { - return _entry; - } - } -} From c4c19441bdbe08da19ee42e7520d5b59ad5ded4e Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Thu, 3 Dec 2020 19:53:00 +0100 Subject: [PATCH 3/4] the first Entry created has no previous entry, so the field is set to null --- .../src/CacheEntryHelper.cs | 2 +- .../tests/MemoryCacheSetAndRemoveTests.cs | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs index 6877d08b7b1a64..50d755699ef039 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs @@ -25,7 +25,7 @@ internal static CacheEntry EnterScope(CacheEntry current) internal static void ExitScope(CacheEntry current, CacheEntry previous) { - Debug.Assert(Current == current); + Debug.Assert(Current == current, "Entries disposed in invalid order"); Current = previous; } } diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs index e9ab2c5c26f502..e9e7fb4ebfc552 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs @@ -214,17 +214,23 @@ public void DisposingCacheEntryReleasesScope() object GetScope(ICacheEntry entry) { return entry.GetType() - .GetField("_scope", BindingFlags.NonPublic | BindingFlags.Instance) + .GetField("_previous", BindingFlags.NonPublic | BindingFlags.Instance) .GetValue(entry); } var cache = CreateCache(); - ICacheEntry entry = cache.CreateEntry("myKey"); - Assert.NotNull(GetScope(entry)); + ICacheEntry first = cache.CreateEntry("myKey1"); + Assert.Null(GetScope(first)); // it's the first entry, so it has no previous cache entry set - entry.Dispose(); - Assert.Null(GetScope(entry)); + ICacheEntry second = cache.CreateEntry("myKey2"); + Assert.NotNull(GetScope(second)); // it's not first, so it has previous set + Assert.Same(first, GetScope(second)); // second.previous is set to first + + second.Dispose(); + Assert.Null(GetScope(second)); + first.Dispose(); + Assert.Null(GetScope(first)); } [Fact] From e6fd38f47ac650dec541d4ff39d8e38b980de33b Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 7 Dec 2020 19:50:35 +0100 Subject: [PATCH 4/4] Update src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs Co-authored-by: Eric Erhardt --- .../Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs index 50d755699ef039..e06ee0fb0100ea 100644 --- a/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs +++ b/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs @@ -6,7 +6,7 @@ namespace Microsoft.Extensions.Caching.Memory { - internal class CacheEntryHelper + internal static class CacheEntryHelper { private static readonly AsyncLocal _current = new AsyncLocal();