Adopting TimeProvider in MemoryCache

[ErisApps]

Hii,

I've been using TimeProvider (and it's unit testing equivalent) for some time now and noticed recently that MemoryCache still uses the ISystemClock, which I thought was deprecated (In reality, it's most likely a copy of it's equivalent in the ASP.NET Core repo, as it's part of Microsoft.Extensions.Caching.Abstractions).

I know it's perfectly possible to just implement a proxy from ISystemClock towards TimeProvider, but I was wondering whether there were any plans to actually deprecate that remaining symbol and transition MemoryCache towards TimeProvider in the BCL altogether? Especially since HybridCache already uses TimeProvider.

[jeffhandley]

This seems like a reasonable request that needs to be turned into an API proposal that marks the existing property as obsolete and adds a new property based on TimeProvider.

[mikocot]

@ErisApps the HybridCache already uses TimeProvider but it seems like it does not even pass it in any way to the MemoryCache, fixing it at the memory cache level as you suggest would definitely make such integrations easier:

see: dotnet/extensions#6646

[ErisApps]

@jeffhandley I'm not sure whether the API design proposal was expected from my end.

But if I need to do a proposal, I'd propose the following:

 public class MemoryCacheOptions
 {
     ...

     /// <summary>
     /// Gets or sets the clock used by the cache for expiration.
     /// </summary>
+    [Obsolete("Please use TimeProvider property instead. If both Clock and TimeProvider are set, TimeProvider will be used for expiration.")]
     public ISystemClock? Clock { get; set; }

+    /// <summary>
+    /// Gets or sets the time provider used by the cache for expiration.
+    /// </Summary>
+    public TimeProvider? TimeProvider { get; set; }

     ...
 }

Subsequently, in the underlying implementation of MemoryCache, we could do something along the lines of:

 public class MemoryCache
 {
     ...
    
-    private DateTime UtcNow => _options.Clock?.UtcNow.UtcDateTime ?? DateTime.UtcNow;
+    private DateTime UtcNow => _options.TimeProvider?.GetUtcNow().UtcDateTime ?? _options.Clock?.UtcNow.UtcDateTime ?? DateTime.UtcNow;

     ...
 }

However, I do realize that this is just a simple replacement which might incur a bit more overhead on every check for UtcNow within the MemoryCache implemention.
Maybe it's more beneficial to write an ISystemClock adapter for TimeProvider alongside the MemoryCacheOptions implementation change instead to reduce overhead in case the user doesn't specify neither ISystemClock nor TimeProvider?

However, that poses the question instead whether the UtcNow provider is supposed to immutable once the MemoryCache instance has been created? If it can/may be, then it could be possible to just do something the lines of this in the MemoryCache implementation instead.

 public class MemoryCache
 {
     ...

+    private readonly TimeProvider _timeProvider;

     ...
    
     public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor, ILoggerFactory loggerFactory)
     {
         ArgumentNullException.ThrowIfNull(optionsAccessor);
         ArgumentNullException.ThrowIfNull(loggerFactory);

         _options = optionsAccessor.Value;
         _logger = loggerFactory.CreateLogger<MemoryCache>();
        
+       if (_options.TimeProvider != null)
+       {
+           _timeProvider = _options.TimeProvider;
+       }
+       else if (_options.Clock != null)
+       {
+           _timeProvider = new SystemClockTimeProviderAdapter(_options.Clock);
+       }
+       else
+       {
+           _timeProvider = TimeProvider.System;
+       }

         ...
     }

-    private DateTime UtcNow => _options.Clock?.UtcNow.UtcDateTime ?? DateTime.UtcNow;
+    private DateTime UtcNow => _timeProvider.GetUtcNow().UtcDateTime;

     ...
 }

and

+ /// <summary>
+ /// An adapter used for switching over from ISystemClock to TimeProvider.
+ /// The primary use case of this class usage of this class is intended for use within MemoryCache
+ /// </summary>
+ internal sealed class SystemClockTimeProviderAdapter : TimeProvider
+ {
+     private readonly ISystemClock _clock;
+
+     public SystemClockTimeProviderAdapter(ISystemClock clock)
+     {
+         ArgumentNullException.ThrowIfNull(clock);
+         _clock = clock;
+     }
+
+     /// <inheritdoc />
+     public override DateTimeOffset GetUtcNow() => _clock.UtcNow;
+
+     /// <inheritdoc />
+     public override long GetTimestamp() => throw new NotSupportedException();
+
+     /// <inheritdoc />
+     public override TimeZoneInfo LocalTimeZone => throw new NotSupportedException();
+
+     /// <inheritdoc />
+     public override long TimestampFrequency => throw new NotSupportedException();
+
+     /// <inheritdoc />
+     public override ITimer CreateTimer(TimerCallback callback, object state, TimeSpan dueTime, TimeSpan period) => throw new NotSupportedException();
+ }

Please note that I throw a NotSupportedException for the properties and methods of that aren't exepected to be used within the MemoryCache implementation.

Please let me know if any further action is required from my end. Just fyi, I wouldn't mind attempting to contribute a PR for this if the proposal gets accepted. 🙂