Fix reliability of a couple of caching tests (#359)

These tests relied on some timing for file writes and static property access. This causes a race condition where the change monitor is disposed at some point internally, but at the same time the test tries to dispose it. This causes the list of change monitors to be enumerated/modified at the same time leading to a crash. The following are fixes/mitigations:

-  Used a TaskCompletionSource for better management of timing in the test to ensure we wait the right amount of time
- Added a lock in the dispose method to guard against reentrancy

This also changed the IHttpRuntime access to go through HttpContext.Current instead of a static property. While not necessary for the current bug, it has been the source of other issues where a test will set the static property at the same time another test is using it. Switching to HttpContext.Current will prevent similar issues as the IHttpRuntime will be request/test local.

Author: twsouthwick

src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/SystemWebAdaptersExtensions.cs

@@ -55,8 +55,6 @@ public static void UseSystemWebAdapters(this IApplicationBuilder app)
     {
         ArgumentNullException.ThrowIfNull(app);
 
-        HttpRuntime.Current = app.ApplicationServices.GetRequiredService<IHttpRuntime>();
-
         app.UseSystemWebAdapterFeatures();
         app.UseAuthenticationEvents();
         app.UseAuthorizationEvents();

src/Microsoft.AspNetCore.SystemWebAdapters/Caching/CacheDependency.cs

@@ -31,10 +31,12 @@ public CacheDependency(string[] filenames, DateTime start) : this(filenames, nul
     public CacheDependency(string[]? filenames, string[]? cachekeys) : this(filenames, cachekeys, null, DateTime.MaxValue) { }
 
     public CacheDependency(string[]? filenames, string[]? cachekeys, DateTime start) :
-        this(filenames, cachekeys, null, start) { }
+        this(filenames, cachekeys, null, start)
+    { }
 
     public CacheDependency(string[]? filenames, string[]? cachekeys, CacheDependency? dependency) :
-        this(filenames, cachekeys, dependency, DateTime.MaxValue) { }
+        this(filenames, cachekeys, dependency, DateTime.MaxValue)
+    { }
 
     public CacheDependency(
         string[]? filenames,
@@ -70,7 +72,7 @@ public CacheDependency(
     protected internal void FinishInit()
     {
         HasChanged = changeMonitors.Any(cm => cm.HasChanged && (cm.GetLastModifiedUtc() > utcStart));
-        utcLastModified = changeMonitors.Count==0 ? DateTime.MinValue : changeMonitors.Max(cm => cm.GetLastModifiedUtc());
+        utcLastModified = changeMonitors.Count == 0 ? DateTime.MinValue : changeMonitors.Max(cm => cm.GetLastModifiedUtc());
         if (HasChanged)
         {
             NotifyDependencyChanged(this, EventArgs.Empty);
@@ -131,19 +133,27 @@ protected virtual void DependencyDispose() { }
 
     protected virtual void Dispose(bool disposing)
     {
-        if (!disposedValue)
+        if (disposing)
         {
-            if (disposing)
+            if (!disposedValue)
             {
-                foreach (var changeMonitor in changeMonitors)
+                // Ensure that if the Dispose is called by different threads (i.e. the wrapping ChangeMonitor) that it won't enumerate the list at the same time
+                lock (changeMonitors)
                 {
-                    changeMonitor?.Dispose();
+                    if (!disposedValue)
+                    {
+                        foreach (var changeMonitor in changeMonitors)
+                        {
+                            changeMonitor?.Dispose();
+                        }
+                        changeMonitors.Clear();
+
+                        DependencyDispose();
+
+                        disposedValue = true;
+                    }
                 }
-                changeMonitors.Clear();
-
-                DependencyDispose();
             }
-            disposedValue = true;
         }
     }
 

src/Microsoft.AspNetCore.SystemWebAdapters/HttpRuntime.cs

@@ -2,22 +2,17 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using Microsoft.AspNetCore.SystemWebAdapters;
+using Microsoft.Extensions.DependencyInjection;
 
 namespace System.Web;
 
 public sealed class HttpRuntime
 {
-    private static IHttpRuntime? _current;
-
     /// <summary>
     /// Gets the current <see cref="IHttpRuntime"/>. This should not be used internally besides where is strictly necessary.
     /// If this is needed, it should be retrieved through dependency injection.
     /// </summary>
-    internal static IHttpRuntime Current
-    {
-        get => _current ?? throw new InvalidOperationException("HttpRuntime is not available in the current environment");
-        set => _current = value;
-    }
+    internal static IHttpRuntime Current => HttpContext.Current.UnwrapAdapter()?.RequestServices.GetRequiredService<IHttpRuntime>() ?? throw new InvalidOperationException("No runtime is currently available");
 
     private HttpRuntime()
     {

test/Microsoft.AspNetCore.SystemWebAdapters.Tests/Caching/CacheTests.cs

@@ -3,9 +3,11 @@
 
 using System.Collections;
 using System.Collections.Generic;
+using System.IO;
 using System.Runtime.Caching;
 using System.Threading.Tasks;
 using AutoFixture;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.SystemWebAdapters;
 using Moq;
 using Xunit;
@@ -340,6 +342,8 @@ public async Task DependentFileCallback()
         var slidingExpiration = TimeSpan.FromMilliseconds(1);
         CacheItemUpdateReason? updateReason = default;
 
+        var tcs = new TaskCompletionSource();
+
         void Callback(string key, CacheItemUpdateReason reason, out object? expensiveObject, out CacheDependency? dependency, out DateTime absoluteExpiration, out TimeSpan slidingExpiration)
         {
             expensiveObject = null;
@@ -349,23 +353,25 @@ void Callback(string key, CacheItemUpdateReason reason, out object? expensiveObj
 
             updated = true;
             updateReason = reason;
+
+            tcs.SetResult();
         }
 
-        var file = System.IO.Path.GetTempFileName();
-        await System.IO.File.WriteAllTextAsync(file, key);
+        var file = Path.GetTempFileName();
+        await File.WriteAllTextAsync(file, key);
 
         using var cd = new CacheDependency(file);
         // Act
         cache.Insert(key, item, cd, Cache.NoAbsoluteExpiration, Cache.NoSlidingExpiration, Callback);
 
         // Ensure file is updated
-        await System.IO.File.WriteAllTextAsync(file, DateTime.UtcNow.ToString("O"));
+        await File.WriteAllTextAsync(file, DateTime.UtcNow.ToString("O"));
 
-        // Small delay here to ensure that the file change notification happens (may fail tests if too fast)
-        await Task.Delay(10);
+        // Wait for callback to be called
+        await tcs.Task;
 
-        // Force cleanup to initiate callbacks on current thread
-        memCache.Trim(100);
+        // Wait for callback to be finalized
+        await Task.Delay(100);
 
         // Assert
         Assert.True(updated);
@@ -382,7 +388,8 @@ public async Task DependentItemCallback()
         var cache = new Cache(memCache);
         var httpRuntime = new Mock<IHttpRuntime>();
         httpRuntime.Setup(s => s.Cache).Returns(cache);
-        HttpRuntime.Current = httpRuntime.Object;
+
+        SetHttpRuntime(httpRuntime.Object);
 
         var item1 = new object();
         var item2 = new object();
@@ -425,4 +432,28 @@ void Callback(string key, CacheItemUpdateReason reason, out object? expensiveObj
         Assert.Equal(CacheItemUpdateReason.Expired, updateReason[key1]);
         Assert.Equal(CacheItemUpdateReason.DependencyChanged, updateReason[key2]);
     }
+
+    private static void SetHttpRuntime(IHttpRuntime runtime)
+    {
+        _ = new HttpContextAccessor
+        {
+            HttpContext = new DefaultHttpContext
+            {
+                RequestServices = new RuntimeServiceProvider(runtime)
+            }
+        };
+    }
+
+    private sealed class RuntimeServiceProvider : IServiceProvider
+    {
+        private readonly IHttpRuntime _runtime;
+
+        public RuntimeServiceProvider(IHttpRuntime runtime)
+        {
+            _runtime = runtime;
+        }
+
+        public object? GetService(Type serviceType)
+            => serviceType == typeof(IHttpRuntime) ? _runtime : null;
+    }
 }