dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.65k stars 10.07k forks source link

Can't modify OutputCacheContext.Tags from IOutputCachePolicy.ServeResponseAsync #46243

Open DanielStout5 opened 1 year ago

DanielStout5 commented 1 year ago

Is there an existing issue for this?

Describe the bug

Assigning tags in ServeResponseAsync doesn't work; the tags don't get attached to the entries, so retrieving or evicting them does nothing.

It makes sense to be able to add tags to the cache entry in ServeResponseAsync (in a custom IOutputCachePolicy).

Work might be done during the request which resolves the value to be used in the tag, which is why it can't be assigned in CacheRequestAsync before the request has actually executed.

I believe the source of the issue is this block in OutputCacheMiddleware:

// Hook up to listen to the response stream
ShimResponseStream(context);

try
{
    await _next(httpContext);

    // The next middleware might change the policy
    foreach (var policy in policies)
    {
        await policy.ServeResponseAsync(context, httpContext.RequestAborted);
    }

    // If there was no response body, check the response headers now. We can cache things like redirects.
    StartResponse(context);

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

ShimResponseStream triggers FinalizeCacheHeaders which creates a copy of the tags (ToArray()) before the request gets executed in await _next - then even though the policy's ServeResponseAsync is using the request data to modify the tags, they don't get persisted by the call to FinalizeCacheBodyAsync, which uses the copy of the tags made in ShimResponseStream.

For a fix, what about removing this line in FinalizeCacheHeaders: Tags = context.Tags.ToArray()

And adding this one in FinalizeCacheBodyAsync: context.CachedResponse.Tags = context.Tags.ToArray();

For now I'm working around it by using reflection to set the OutputCacheContext.CachedResponse.Tags array inside ServerResponseAsync - reflection being necessary because CachedResponse (both the property and the class OutputCacheEntry) are internal

Expected Behavior

Assigning tags in ServeResponseAsync should attach them to the stored cache entry.

Steps To Reproduce

Create an implementation of IOutputCachePolicy and add:

        public ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellation)
        {
            context.AllowCacheStorage = true;
            context.Tags.Add("MyTagHere");
            return ValueTask.CompletedTask;
        }

Exceptions (if any)

No response

.NET Version

7.0.101

Anything else?

ASP.NET Core 7.0.2 Visual Studio Professional 2022

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

pecoult commented 1 year ago

Hello, We have the same issue with custom output cache policies.

.NET Version: 7.0.102

The scenario we tried to achieve:

As a fallback we think about using IOutputCachePolicy CacheRequestAsync method but we will only be able to create dynamic tags based on request data. It is not ideal in our case. If you see any other way to manage and purge cache entries based on calculated tags please give advices. I will give a try to your workaround @DanielStout5 with reflection. Thank you!

Sample (simplified code example): Startup.cs:

services.AddOutputCache(c => 
    {
        c.AddBasePolicy(builder => builder.AddPolicy<CustomOutputCachePolicy>().Cache());
    });

CustomOutputCachePolicy.cs

public class CustomOutputCachePolicy : IOutputCachePolicy
    {
        public ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken cancellation) => ValueTask.CompletedTask;

        public ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken cancellation) => ValueTask.CompletedTask;

        public ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellation)
        {
            if (context.HttpContext.Response.Headers.ContainsKey("Cache-Tags"))
            {
                var tag = context.HttpContext.Response.Headers["Cache-Tags"].First();
                context.Tags.Add(tag);
            }
            return ValueTask.CompletedTask;
        }
    }

ResourceController.cs

[...]
        [HttpGet]
        [OutputCache]
        public IActionResult GetResources(string userId)
        {
            var resources = _service.GetResourcesByUserId(userId);

            Response.Headers.Add("Cache-Tags", $"Resources-{userId}");

            return Ok(resources);
        }

        [HttpPost]
        public async Task<IActionResult> CreateResourceAsync(Resource resource, string userId)
        {
            var resourceId = _service.CreateResource(resource, userId);

            await _outputCacheStore.EvictByTagAsync($"Resources-{userId}", CancellationToken.None);
            return Created(uri: resourceId, value: resourceId);
        }
[...]
KathigitAnguras commented 1 year ago

I came across this issue today. Totally agree with @DanielStout5.

I understand setting the headers in StartResponse(), but tags should not be treated the same, considering tags is the only good way to apply eviction. And as far as I can see, generating dynamic tags (that accurately represent the cached entry) can only be done from within the context of the response.

Without the context of the response, we are left with applying generic tags and evict cache entries unnecessarily (this becomes more apparent when the cached output contains other related entities, thus creating more complex eviction scenarios).

In summary, without this, output cache becomes less flexible to accurately applying eviction to ... cached output and considering the relatively low effort to fix this, I am surprised this is not treated with a higher priority.

therealkungfury commented 1 year ago

I also stumbled into this one recently. Had to listen to my co-worker moan about this for two hours :)

It does seem like a very reasonable improvement which would make the output cache a lot more flexible. Got my vote.