[Proposal] use Timer in MemoryCache

src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs

@@ -33,6 +33,7 @@ internal CacheEntry(object key, MemoryCache memoryCache)
             Key = key ?? throw new ArgumentNullException(nameof(key));
             _cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));
             _previous = CacheEntryHelper.EnterScope(this);
+            _state = (int)State.CanExpire;
         }
 
         /// <summary>
@@ -133,9 +134,21 @@ public object Value
 
         internal EvictionReason EvictionReason { get; private set; }
 
+        internal bool CanExpire
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get => ((State)_state).HasFlag(State.CanExpire);
+            set => Set(State.CanExpire, value);
+        }
+
         private bool IsDisposed { get => ((State)_state).HasFlag(State.IsDisposed); set => Set(State.IsDisposed, value); }
 
-        private bool IsExpired { get => ((State)_state).HasFlag(State.IsExpired); set => Set(State.IsExpired, value); }
+        private bool IsExpired
+        {
+            [MethodImpl(MethodImplOptions.AggressiveInlining)]
+            get => ((State)_state).HasFlag(State.IsExpired);
+            set => Set(State.IsExpired, value);
+        }
 
         private bool IsValueSet { get => ((State)_state).HasFlag(State.IsValueSet); set => Set(State.IsValueSet, value); }
 
@@ -150,6 +163,9 @@ public void Dispose()
                 // so don't use this entry.
                 if (IsValueSet)
                 {
+                    // set the CanExpire flag before adding the entry to the cache
+                    CanExpire = AbsoluteExpiration.HasValue || SlidingExpiration.HasValue || AbsoluteExpirationRelativeToNow.HasValue || _expirationTokens != null;
+
                     _cache.SetEntry(this);
 
                     if (_previous != null && CanPropagateOptions())
@@ -164,7 +180,7 @@ public void Dispose()
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal bool CheckExpired(in DateTimeOffset now) => IsExpired || CheckForExpiredTime(now) || CheckForExpiredTokens();
+        internal bool CheckExpired(DateTimeOffset now) => IsExpired || (CanExpire && (CheckForExpiredTime(now) || CheckForExpiredTokens()));
 
         internal void SetExpired(EvictionReason reason)
         {
@@ -176,32 +192,22 @@ internal void SetExpired(EvictionReason reason)
             DetachTokens();
         }
 
-        private bool CheckForExpiredTime(in DateTimeOffset now)
+        private bool CheckForExpiredTime(DateTimeOffset now)
         {
-            if (!AbsoluteExpiration.HasValue && !_slidingExpiration.HasValue)
+            if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= now)
             {
-                return false;
+                SetExpired(EvictionReason.Expired);
+                return true;
             }
 
-            return FullCheck(now);
-
-            bool FullCheck(in DateTimeOffset offset)
+            if (_slidingExpiration.HasValue
+                && (now - LastAccessed) >= _slidingExpiration)
             {
-                if (AbsoluteExpiration.HasValue && AbsoluteExpiration.Value <= offset)
-                {
-                    SetExpired(EvictionReason.Expired);
-                    return true;
-                }
-
-                if (_slidingExpiration.HasValue
-                    && (offset - LastAccessed) >= _slidingExpiration)
-                {
-                    SetExpired(EvictionReason.Expired);
-                    return true;
-                }
-
-                return false;
+                SetExpired(EvictionReason.Expired);
+                return true;
             }
+
+            return false;
         }
 
         private bool CheckForExpiredTokens()
@@ -315,6 +321,7 @@ private static void InvokeCallbacks(CacheEntry entry)
         }
 
         // this simple check very often allows us to avoid expensive call to PropagateOptions(CacheEntryHelper.Current)
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal bool CanPropagateOptions() => _expirationTokens != null || AbsoluteExpiration.HasValue;
 
         internal void PropagateOptions(CacheEntry parent)
@@ -336,6 +343,7 @@ internal void PropagateOptions(CacheEntry parent)
                         {
                             parent.AddExpirationToken(expirationToken);
                         }
+                        parent.CanExpire = true;
                     }
                 }
             }
@@ -345,6 +353,7 @@ internal void PropagateOptions(CacheEntry parent)
                 if (!parent.AbsoluteExpiration.HasValue || AbsoluteExpiration < parent.AbsoluteExpiration)
                 {
                     parent.AbsoluteExpiration = AbsoluteExpiration;
+                    parent.CanExpire = true;
                 }
             }
         }
@@ -367,6 +376,7 @@ private enum State
             IsValueSet = 1 << 0,
             IsExpired = 1 << 1,
             IsDisposed = 1 << 2,
+            CanExpire = 1 << 3,
         }
     }
 }

src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs

@@ -4,9 +4,7 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Runtime.CompilerServices;
 using System.Threading;
-using System.Threading.Tasks;
 using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
@@ -24,10 +22,10 @@ public class MemoryCache : IMemoryCache
 
         private readonly MemoryCacheOptions _options;
         private readonly ConcurrentDictionary<object, CacheEntry> _entries;
+        private readonly System.Timers.Timer _timer;
 
         private long _cacheSize;
         private bool _disposed;
-        private DateTimeOffset _lastExpirationScan;
 
         /// <summary>
         /// Creates a new <see cref="MemoryCache"/> instance.
@@ -63,7 +61,9 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory
                 _options.Clock = new SystemClock();
             }
 
-            _lastExpirationScan = _options.Clock.UtcNow;
+            _timer = new System.Timers.Timer(Math.Max(1, _options.ExpirationScanFrequency.TotalMilliseconds));
+            _timer.Elapsed += OnTimerElapsed;
+            _timer.Start();
         }
 
         /// <summary>
@@ -211,8 +211,6 @@ internal void SetEntry(CacheEntry entry)
                     RemoveEntry(priorEntry);
                 }
             }
-
-            StartScanForExpiredItemsIfNeeded(utcNow);
         }
 
         /// <inheritdoc />
@@ -221,39 +219,34 @@ public bool TryGetValue(object key, out object result)
             ValidateCacheKey(key);
             CheckDisposed();
 
-            DateTimeOffset utcNow = _options.Clock.UtcNow;
+            if (!_entries.TryGetValue(key, out CacheEntry entry))
+            {
+                result = null;
+                return false;
+            }
 
-            if (_entries.TryGetValue(key, out CacheEntry entry))
+            if (entry.CanExpire)
             {
-                // Check if expired due to expiration tokens, timers, etc. and if so, remove it.
-                // Allow a stale Replaced value to be returned due to concurrent calls to SetExpired during SetEntry.
-                if (!entry.CheckExpired(utcNow) || entry.EvictionReason == EvictionReason.Replaced)
+                DateTimeOffset utcNow = _options.Clock.UtcNow;
+                if (entry.EvictionReason != EvictionReason.Replaced && entry.CheckExpired(utcNow))
                 {
-                    entry.LastAccessed = utcNow;
-                    result = entry.Value;
-
-                    if (entry.CanPropagateOptions())
-                    {
-                        // When this entry is retrieved in the scope of creating another entry,
-                        // that entry needs a copy of these expiration tokens.
-                        entry.PropagateOptions(CacheEntryHelper.Current);
-                    }
-
-                    StartScanForExpiredItemsIfNeeded(utcNow);
+                    RemoveEntry(entry);
 
-                    return true;
+                    result = null;
+                    return false;
                 }
-                else
+
+                entry.LastAccessed = utcNow;
+                if (entry.CanPropagateOptions())
                 {
-                    // TODO: For efficiency queue this up for batch removal
-                    RemoveEntry(entry);
+                    // When this entry is retrieved in the scope of creating another entry,
+                    // that entry needs a copy of these expiration tokens.
+                    entry.PropagateOptions(CacheEntryHelper.Current);
                 }
             }
 
-            StartScanForExpiredItemsIfNeeded(utcNow);
-
-            result = null;
-            return false;
+            result = entry.Value;
+            return true;
         }
 
         /// <inheritdoc />
@@ -272,8 +265,6 @@ public void Remove(object key)
                 entry.SetExpired(EvictionReason.Removed);
                 entry.InvokeEvictionCallbacks();
             }
-
-            StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow);
         }
 
         private void RemoveEntry(CacheEntry entry)
@@ -292,30 +283,13 @@ internal void EntryExpired(CacheEntry entry)
         {
             // TODO: For efficiency consider processing these expirations in batches.
             RemoveEntry(entry);
-            StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow);
         }
 
-        // Called by multiple actions to see how long it's been since we last checked for expired items.
-        // If sufficient time has elapsed then a scan is initiated on a background task.
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private void StartScanForExpiredItemsIfNeeded(DateTimeOffset utcNow)
-        {
-            if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan)
-            {
-                ScheduleTask(utcNow);
-            }
-
-            void ScheduleTask(DateTimeOffset utcNow)
-            {
-                _lastExpirationScan = utcNow;
-                Task.Factory.StartNew(state => ScanForExpiredItems((MemoryCache)state), this,
-                    CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
-            }
-        }
+        private void OnTimerElapsed(object sender, System.Timers.ElapsedEventArgs e) => ScanForExpiredItems(this);
 
         private static void ScanForExpiredItems(MemoryCache cache)
         {
-            DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow;
+            DateTimeOffset now = cache._options.Clock.UtcNow;
             foreach (CacheEntry entry in cache._entries.Values)
             {
                 if (entry.CheckExpired(now))
@@ -483,6 +457,10 @@ protected virtual void Dispose(bool disposing)
                     GC.SuppressFinalize(this);
                 }
 
+                _timer.Stop();
+                _timer.Elapsed -= OnTimerElapsed;
+                _timer.Dispose();
+
                 _disposed = true;
             }
         }

src/libraries/Microsoft.Extensions.Caching.Memory/tests/CacheEntryScopeExpirationTests.cs

@@ -8,23 +8,12 @@
 using Microsoft.Extensions.Internal;
 using Xunit;
 
+using static Microsoft.Extensions.Internal.CacheFactory;
+
 namespace Microsoft.Extensions.Caching.Memory
 {
     public class CacheEntryScopeExpirationTests
     {
-        private IMemoryCache CreateCache()
-        {
-            return CreateCache(new SystemClock());
-        }
-
-        private IMemoryCache CreateCache(ISystemClock clock)
-        {
-            return new MemoryCache(new MemoryCacheOptions()
-            {
-                Clock = clock,
-            });
-        }
-
         [Fact]
         public void SetPopulates_ExpirationTokens_IntoScopedLink()
         {

src/libraries/Microsoft.Extensions.Caching.Memory/tests/Infrastructure/CacheFactory.cs

@@ -0,0 +1,36 @@
+// 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 Microsoft.Extensions.Caching.Memory;
+
+namespace Microsoft.Extensions.Internal
+{
+    internal static class CacheFactory
+    {
+        internal static IMemoryCache CreateCache()
+        {
+            return CreateCache(new SystemClock());
+        }
+
+        internal static IMemoryCache CreateCache(ISystemClock clock)
+        {
+            return new MemoryCache(new MemoryCacheOptions()
+            {
+                Clock = clock,
+            });
+        }
+
+        internal static IMemoryCache CreateCache(ISystemClock clock, TimeSpan expirationScanFrequency)
+        {
+            var options = new MemoryCacheOptions()
+            {
+                Clock = clock,
+                ExpirationScanFrequency = expirationScanFrequency,
+            };
+
+            return new MemoryCache(options);
+        }
+    }
+}

src/libraries/Microsoft.Extensions.Caching.Memory/tests/TimeExpirationTests.cs

@@ -6,18 +6,12 @@
 using Microsoft.Extensions.Internal;
 using Xunit;
 
+using static Microsoft.Extensions.Internal.CacheFactory;
+
 namespace Microsoft.Extensions.Caching.Memory
 {
     public class TimeExpirationTests
     {
-        private IMemoryCache CreateCache(ISystemClock clock)
-        {
-            return new MemoryCache(new MemoryCacheOptions()
-            {
-                Clock = clock,
-            });
-        }
-
         [Fact]
         public void AbsoluteExpirationInThePastDoesntAddEntry()
         {
@@ -58,7 +52,8 @@ public void AbsoluteExpirationExpires()
         public void AbsoluteExpirationExpiresInBackground()
         {
             var clock = new TestClock();
-            var cache = CreateCache(clock);
+            TimeSpan expirationScanFrequency = TimeSpan.FromMilliseconds(100);
+            var cache = CreateCache(clock, expirationScanFrequency);
             var key = "myKey";
             var value = new object();
             var callbackInvoked = new ManualResetEvent(false);
@@ -79,8 +74,6 @@ public void AbsoluteExpirationExpiresInBackground()
             Assert.Same(value, result);
 
             clock.Add(TimeSpan.FromMinutes(2));
-            var ignored = cache.Get("otherKey"); // Background expiration checks are triggered by misc cache activity.
-
             Assert.True(callbackInvoked.WaitOne(TimeSpan.FromSeconds(30)), "Callback");
 
             found = cache.TryGetValue(key, out result);