[main] Critical fix to output cache; remove OutputCacheEntry recycling (#51348)

* criticial fix to output cache; remove OutputCacheEntry recycling; see #51009 (nit: ensure Headers is consistently empty if CopyHeadersFrom is no-op)

* remove _recycleBuffers

* make IDE0060 happy

* don't swallow exception!

* regression test for output cache middleware fault

* disable array-pool usage in output-cache

---------

Co-authored-by: Marc Gravell <[email protected]>

Author: github-actions[bot]

src/Middleware/OutputCaching/src/OutputCacheEntry.cs

@@ -3,7 +3,6 @@
 
 using System.Buffers;
 using System.IO.Pipelines;
-using System.Runtime.InteropServices;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.Primitives;
 using Microsoft.Net.Http.Headers;
@@ -18,8 +17,6 @@ public OutputCacheEntry(DateTimeOffset created, int statusCode)
         StatusCode = statusCode;
     }
 
-    private bool _recycleBuffers; // does this instance own the memory behind the segments?
-
     public StringValues FindHeader(string key)
     {
         TryFindHeader(key, out var value);
@@ -69,26 +66,9 @@ public bool TryFindHeader(string key, out StringValues values)
     internal void SetBody(ReadOnlySequence<byte> value, bool recycleBuffers)
     {
         Body = value;
-        _recycleBuffers = recycleBuffers;
-    }
-
-    public void Dispose()
-    {
-        var headers = Headers;
-        var body = Body;
-        Headers = default;
-        Body = default;
-        Recycle(headers);
-        RecyclableReadOnlySequenceSegment.RecycleChain(body, _recycleBuffers);
-        // ^^ note that this only recycles the chain, not the actual buffers
-    }
-
-    private static void Recycle<T>(ReadOnlyMemory<T> value)
-    {
-        if (MemoryMarshal.TryGetArray<T>(value, out var segment) && segment.Array is { Length: > 0 })
-        {
-            ArrayPool<T>.Shared.Return(segment.Array);
-        }
+        _ = recycleBuffers; // satisfy IDE0060
+        // note that recycleBuffers is not stored currently, until OutputCacheEntry buffer recycling is re-implemented;
+        // it indicates whether this instance "owns" the memory behind the segments, such that they can be recycled later if desired
     }
 
     internal OutputCacheEntry CreateBodyFrom(IList<byte[]> segments) // mainly used from tests
@@ -118,6 +98,7 @@ internal OutputCacheEntry CopyHeadersFrom(IHeaderDictionary headers)
                 if (index == 0) // only ignored headers
                 {
                     ArrayPool<(string, StringValues)>.Shared.Return(arr);
+                    Headers = default;
                 }
                 else
                 {
@@ -142,4 +123,6 @@ public void CopyHeadersTo(IHeaderDictionary headers)
 
     public ValueTask CopyToAsync(PipeWriter destination, CancellationToken cancellationToken)
         => RecyclableReadOnlySequenceSegment.CopyToAsync(Body, destination, cancellationToken);
+
+    public void Dispose() { } // intention here is to add recycling; this was removed late in NET8, but is retained as a callback
 }

src/Middleware/OutputCaching/src/OutputCacheMiddleware.cs

@@ -200,6 +200,7 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
         {
             // avoid recycling in unknown outcomes, especially re concurrent buffer access thru cancellation
             hasException = true;
+            throw;
         }
         finally
         {

src/Middleware/OutputCaching/src/RecyclableReadOnlySequenceSegment.cs

@@ -4,17 +4,25 @@
 using System.Buffers;
 using System.Collections.Concurrent;
 using System.IO.Pipelines;
-using System.Runtime.InteropServices;
 
 namespace Microsoft.AspNetCore.OutputCaching;
 
+// TODO: reinstate pooling
+// context: a last-minute bug was detected during net8 preparation that impacted
+// buffer reuse from output-cache (the only consumer of this type); the preferred
+// solution for this is understood, but is more "moving parts" than we're comfortable
+// touching in the last phase of net8, so to avoid risk, *temporarily*, the buffer
+// reuse is disabled; this is consistent with net7, which never used buffer recycling
+// in output-cache, so this is not a regression. The work to properly implement buffer
+// reuse in output-cache is in progress to be merged in net9 and hopefully backported
+// into a net8 service release.
 internal sealed class RecyclableReadOnlySequenceSegment : ReadOnlySequenceSegment<byte>
 {
     public int Length => Memory.Length;
     private RecyclableReadOnlySequenceSegment() { }
 
     public static RecyclableReadOnlySequenceSegment Create(int minimumLength, RecyclableReadOnlySequenceSegment? previous)
-        => Create(Rent(minimumLength), previous);
+        => Create(GetBuffer(minimumLength), previous);
 
     public static RecyclableReadOnlySequenceSegment Create(ReadOnlyMemory<byte> memory, RecyclableReadOnlySequenceSegment? previous)
     {
@@ -112,14 +120,16 @@ public static async ValueTask CopyToAsync(ReadOnlySequence<byte> source, PipeWri
         }
     }
 
-    private static byte[] Rent(int minimumLength)
-        => ArrayPool<byte>.Shared.Rent(minimumLength);
+    // TODO: reinstate ArrayPool<byte>.Shared usage.Rent(minimumLength);
+    private static byte[] GetBuffer(int minimumLength)
+        => new byte[minimumLength];
 
-    private static void Recycle(ReadOnlyMemory<byte> value)
+    private static void Recycle(ReadOnlyMemory<byte> _)
     {
-        if (MemoryMarshal.TryGetArray(value, out var segment) && segment.Offset == 0 && segment.Count != 0)
-        {
-            ArrayPool<byte>.Shared.Return(segment.Array!);
-        }
+        // TODO: reinstate buffer recycling
+        //if (MemoryMarshal.TryGetArray(value, out var segment) && segment.Offset == 0 && segment.Count != 0)
+        //{
+        //    ArrayPool<byte>.Shared.Return(segment.Array!);
+        //}
     }
 }

src/Middleware/OutputCaching/test/OutputCacheTests.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Net;
 using System.Net.Http;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.TestHost;
@@ -935,6 +936,100 @@ public async Task ServesCachedContent_IfAvailable_UsingHead_WithContentLength()
         }
     }
 
+    [Fact]
+    public async Task MiddlewareFaultsAreObserved()
+    {
+        var builders = TestUtils.CreateBuildersWithOutputCaching(contextAction: _ => throw new SomeException());
+
+        foreach (var builder in builders)
+        {
+            using var host = builder.Build();
+
+            await host.StartAsync();
+
+            using var server = host.GetTestServer();
+
+            for (int i = 0; i < 10; i++)
+            {
+                await RunClient(server);
+            }
+        }
+
+        static async Task RunClient(TestServer server)
+        {
+            var client = server.CreateClient();
+            await Assert.ThrowsAsync<SomeException>(
+                () => client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "")));
+        }
+    }
+
+    sealed class SomeException : Exception { }
+
+    [Fact]
+    public async Task ServesCorrectlyUnderConcurrentLoad()
+    {
+        var builders = TestUtils.CreateBuildersWithOutputCaching();
+
+        foreach (var builder in builders)
+        {
+            using var host = builder.Build();
+
+            await host.StartAsync();
+
+            using var server = host.GetTestServer();
+
+            var guid = await RunClient(server, -1);
+
+            var clients = new Task<Guid>[1024];
+            for (int i = 0; i < clients.Length; i++)
+            {
+                clients[i] = Task.Run(() => RunClient(server, i));
+            }
+            await Task.WhenAll(clients);
+
+            // note already completed
+            for (int i = 0; i < clients.Length; i++)
+            {
+                Assert.Equal(guid, await clients[i]);
+            }
+        }
+
+        static async Task<Guid> RunClient(TestServer server, int id)
+        {
+            string s = null;
+            try
+            {
+                var client = server.CreateClient();
+                var resp = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, ""));
+                var len = resp.Content.Headers.ContentLength;
+                s = await resp.Content.ReadAsStringAsync();
+
+                Assert.NotNull(len);
+                Guid value;
+                switch (len.Value)
+                {
+                    case 36:
+                        // usually we just write a guid
+                        Assert.True(Guid.TryParse(s, out value));
+                        break;
+                    case 98:
+                        // the file-based builder prepends extra data
+                        Assert.True(Guid.TryParse(s.Substring(s.Length - 36), out value));
+                        break;
+                    default:
+                        Assert.Fail($"Unexpected length: {len.Value}");
+                        value = Guid.NewGuid(); // not reached
+                        break;
+                }
+                return value;
+            }
+            catch (Exception ex)
+            {
+                throw new InvalidOperationException($"Client {id} failed; payload '{s}', failure: {ex.Message}", ex);
+            }
+        }
+    }
+
     private static void Assert304Headers(HttpResponseMessage initialResponse, HttpResponseMessage subsequentResponse)
     {
         // https://tools.ietf.org/html/rfc7232#section-4.1