Skip to content

[release/10.0] Don't cache truncated/aborted responses in OutputCacheMiddleware #68770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DeagleGross wants to merge 1 commit into
base: release/10.0
Choose a base branch
from
+188 −7
Open

[release/10.0] Don't cache truncated/aborted responses in OutputCacheMiddleware #68770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/Middleware/OutputCaching/src/OutputCacheMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
// Hook up to listen to the response stream
ShimResponseStream(context);

var isResponseCached = false;

try
{
await _next(httpContext);
Expand All @@ -174,7 +176,7 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
StartResponse(context);

// Finalize the cache entry
await FinalizeCacheBodyAsync(context);
isResponseCached = await FinalizeCacheBodyAsync(context);

executed = true;
}
Expand All @@ -183,9 +185,8 @@ private async Task InvokeAwaited(HttpContext httpContext, IReadOnlyList<IOutputC
UnshimResponseStream(context);
}

// If the policies prevented this response from being cached we can't reuse it for other
// pending requests
if (!context.AllowCacheStorage)
// If the response wasn't cached, we can't reuse it for other pending requests
if (!isResponseCached)
{
context.ReleaseCachedResponse();
}
Expand Down Expand Up @@ -411,12 +412,15 @@ internal void FinalizeCacheHeaders(OutputCacheContext context)
}

/// <summary>
/// Stores the response body
/// Attempts to store the response body.
/// </summary>
internal async ValueTask FinalizeCacheBodyAsync(OutputCacheContext context)
/// <param name="context">The <see cref="OutputCacheContext"/>.</param>
/// <returns><c>true</c> if the response was cached; otherwise <c>false</c>.</returns>
internal async ValueTask<bool> FinalizeCacheBodyAsync(OutputCacheContext context)
{
if (context.AllowCacheStorage && context.OutputCacheStream.BufferingEnabled
&& context.CachedResponse is not null)
&& context.CachedResponse is not null
&& !context.HttpContext.RequestAborted.IsCancellationRequested)
{
// If AllowCacheLookup is false, the cache key was not created
CreateCacheKey(context);
Expand All @@ -441,6 +445,8 @@ internal async ValueTask FinalizeCacheBodyAsync(OutputCacheContext context)

await OutputCacheEntryFormatter.StoreAsync(context.CacheKey, context.CachedResponse, context.Tags, context.CachedResponseValidFor,
_store, _logger, context.HttpContext.RequestAborted);

return true;
}
}
else
Expand All @@ -452,6 +458,8 @@ await OutputCacheEntryFormatter.StoreAsync(context.CacheKey, context.CachedRespo
{
_logger.ResponseNotCached();
}

return false;
}

/// <summary>
Expand Down
173 changes: 173 additions & 0 deletions src/Middleware/OutputCaching/test/OutputCacheMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,33 @@ public async Task FinalizeCacheBody_DoNotCache_IfBufferingDisabled()
LoggedMessage.ResponseNotCached);
}

[Fact]
public async Task FinalizeCacheBody_DoNotCache_IfRequestAborted()
{
var cache = GetStore();
var sink = new TestSink();
var middleware = TestUtils.CreateTestMiddleware(testSink: sink, cache: cache);
var context = TestUtils.CreateTestContext(cache: cache);

middleware.ShimResponseStream(context);
await context.HttpContext.Response.WriteAsync(new string('0', 10));

using var entry = new OutputCacheEntry(DateTimeOffset.UtcNow, StatusCodes.Status200OK);
context.CachedResponse = entry;
context.CacheKey = "BaseKey";
context.CachedResponseValidFor = TimeSpan.FromSeconds(10);

context.HttpContext.RequestAborted = new CancellationToken(canceled: true);

var isResponseCached = await middleware.FinalizeCacheBodyAsync(context);

Assert.Equal(0, cache.SetCount);
Assert.False(isResponseCached);
TestUtils.AssertLoggedMessages(
sink.Writes,
LoggedMessage.ResponseNotCached);
}

[Fact]
public async Task FinalizeCacheBody_DoNotCache_IfSizeTooBig()
{
Expand Down Expand Up @@ -912,6 +939,69 @@ public async Task Locking_IgnoresNonCacheableResponses()
Assert.Equal("Hello2", Encoding.UTF8.GetString(memoryStream2.ToArray()));
}

[Fact]
public async Task Locking_IgnoresTruncatedResponses()
{
var responseCounter = 0;
var cache = GetStore();

var blocker1 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);
var blocker2 = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

var memoryStream1 = new MemoryStream();
var memoryStream2 = new MemoryStream();

var options = new OutputCacheOptions();
options.AddBasePolicy(build => build.Cache());

var middleware = TestUtils.CreateTestMiddleware(options: options, cache: cache, next: async c =>
{
responseCounter++;

if (responseCounter == 1)
{
blocker1.SetResult(true);

// Announce more bytes than are written so the response is truncated
c.Response.ContentLength = 1000;
}

c.Response.Write("Hello" + responseCounter);

await blocker2.Task;
});

var context1 = TestUtils.CreateTestContext(cache: cache);
context1.HttpContext.Request.Method = "GET";
context1.HttpContext.Request.Path = "/";
context1.HttpContext.Response.Body = memoryStream1;

var context2 = TestUtils.CreateTestContext(cache: cache);
context2.HttpContext.Request.Method = "GET";
context2.HttpContext.Request.Path = "/";
context2.HttpContext.Response.Body = memoryStream2;

var task1 = Task.Run(() => middleware.Invoke(context1.HttpContext));

// Wait for context1 to be processed
await blocker1.Task;

// Start context2 and let it run until it is blocked by the locking feature
var task2 = middleware.Invoke(context2.HttpContext);
Assert.False(task2.IsCompleted);

// Unblock context1
blocker2.SetResult(true);

await Task.WhenAll(task1, task2);

Assert.Equal(2, responseCounter);

// Ensure the truncated response was not returned from cache
Assert.Equal("Hello1", Encoding.UTF8.GetString(memoryStream1.ToArray()));
Assert.Equal("Hello2", Encoding.UTF8.GetString(memoryStream2.ToArray()));
}

[Fact]
[QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/55652")]
public async Task Locking_ExecuteAllRequestsWhenDisabled()
Expand Down Expand Up @@ -960,6 +1050,89 @@ public async Task Locking_ExecuteAllRequestsWhenDisabled()
Assert.Equal(2, responseCounter);
}

[Fact]
public async Task AbortedRequest_DoesNotReExecuteRequest()
{
var responseCounter = 0;
var cache = GetStore();

var options = new OutputCacheOptions();
options.AddBasePolicy(build => build.Cache());

var middleware = TestUtils.CreateTestMiddleware(options: options, cache: cache, next: c =>
{
responseCounter++;

c.Response.Write("Hello" + responseCounter);

// Simulates an action result that aborts after a canceled write
c.RequestAborted = new CancellationToken(canceled: true);

return Task.CompletedTask;
});

var memoryStream = new MemoryStream();

var context = TestUtils.CreateTestContext(cache: cache);
context.HttpContext.Request.Method = "GET";
context.HttpContext.Request.Path = "/";
context.HttpContext.Response.Body = memoryStream;

await middleware.Invoke(context.HttpContext);

// The aborted request must not run the pipeline a second time
Assert.Equal(1, responseCounter);
Assert.Equal(0, cache.SetCount);
Assert.Equal("Hello1", Encoding.UTF8.GetString(memoryStream.ToArray()));
}

[Fact]
public async Task AbortedRequest_IsNotServedToSubsequentRequests()
{
var responseCounter = 0;
var cache = GetStore();

var options = new OutputCacheOptions();
options.AddBasePolicy(build => build.Cache());

var middleware = TestUtils.CreateTestMiddleware(options: options, cache: cache, next: c =>
{
responseCounter++;

c.Response.Write("Hello" + responseCounter);

if (responseCounter == 1)
{
c.RequestAborted = new CancellationToken(canceled: true);
}

return Task.CompletedTask;
});

var memoryStream1 = new MemoryStream();

var context1 = TestUtils.CreateTestContext(cache: cache);
context1.HttpContext.Request.Method = "GET";
context1.HttpContext.Request.Path = "/";
context1.HttpContext.Response.Body = memoryStream1;

await middleware.Invoke(context1.HttpContext);

var memoryStream2 = new MemoryStream();

var context2 = TestUtils.CreateTestContext(cache: cache);
context2.HttpContext.Request.Method = "GET";
context2.HttpContext.Request.Path = "/";
context2.HttpContext.Response.Body = memoryStream2;

await middleware.Invoke(context2.HttpContext);

// The later request runs its own delegate, not the aborted response
Assert.Equal(2, responseCounter);
Assert.Equal("Hello1", Encoding.UTF8.GetString(memoryStream1.ToArray()));
Assert.Equal("Hello2", Encoding.UTF8.GetString(memoryStream2.ToArray()));
}

[Fact]
public async Task EmptyCacheKey_IsNotCached()
{
Expand Down
Loading