Add IOutputCacheBufferWriterStore

[mgravell]

Ideally for preview 6...

Background and Motivation

We wish to implement an official SE.Redis output cache implementation.

The existing IOutputCacheStore API is byte[]-focused, which is fine for the in-process implementation which constantly returns the same byte[], but is hugely suboptimal for out-of-process implementations, as we would have to return a right-sized byte[] every call (in either direction), causing GC load. Additionally, since output-cache is an entire page load plus headers, this can easily be large enough to spill into LOH.

Context: #48450

IMPORTANT: this API was approved for RC6, but an unforeseen problem arose (also #50402) where-by this change added a framework reference into a previously non-framework-reference OOB package; this is undesirable, so a secondary change is proposed to resolve this. The obsolete parts of this proposal have been struck, and the new changes are in italics.

For this, we propose three things:

1. a new SetAsync method similar to the existing, but taking ReadOnlySequence<byte> instead of byte[]; this API to be implemented in the existing IOutputCacheStore interface as a DIM, to avoid back-compat problems
2. a new interface IOutputCacheBufferWriterStore which extends IOutputCacheStore (same assembly/namespace), allowing implementations to optionally use a new GetAsync API that takes an IBufferWriter<byte> - thus allowing implementations to pass data to the consumer while site-stepping the topic of "data lifetime" : the receiver can pass a recycling-enabled buffer-writer, and the cache implementation does not need to know about those details - it just asks for space to write data, and writes
3. a new API to register the SE.Redis output cache implementation, in the existing Microsoft.Extensions.Caching.StackExchangeRedis OOB package in a new Microsoft.AspNetCore.OutputCaching.StackExchangeRedis OOB package

Proposed API

  namespace Microsoft.AspNetCore.OutputCaching;

  public interface IOutputCacheStore
  {
      // pre-existing
      ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken);

    
+     ValueTask SetAsync(string key, ReadOnlySequence<byte> value, ReadOnlyMemory<string> tags, TimeSpan validFor, CancellationToken cancellationToken)
+     {
+         // compatibility implementation using the original API
+         return SetAsync(key, value.ToArray(), tags.IsEmpty ? null : tags.ToArray(), validFor, cancellationToken);
+     }
  }

+ public interface IOutputCacheBufferWriterStore : IOutputCacheStore
+ {
+     ValueTask<bool> GetAsync(string key, IBufferWriter<byte> destination, CancellationToken cancellationToken);
+ }

This second interface is optional / separate because not all types will wish to opt in; in particular, in the case of the existing in-proc implementation, it will be preferable to just keep returning the existing byte[]; as such, the output cache middleware will test for this API and use the most appropriate.

Additionally, to implement the SE.Redis cache, we propose the addition of

package: `Microsoft.Extensions.Caching.StackExchangeRedis` (pre-existing) type `StackExchangeRedisCacheServiceCollectionExtensions` (pre-existing)

+ public static IServiceCollection AddStackExchangeRedisOutputCache(
+     this IServiceCollection services, Action<RedisCacheOptions> setupAction)

package: Microsoft.AspNetCore.OutputCaching.StackExchangeRedis (new)
type Microsoft.AspNetCore.OutputCaching.StackExchangeRedis (new)

+ public static IServiceCollection AddStackExchangeRedisOutputCache(
+     this IServiceCollection services, Action<RedisOutputCacheOptions> setupAction)

where RedisOutputCacheOptions is a new clone of the existing RedisCacheOptions, but in the new package/namespace (otherwise: 1-1 feature parity with RedisCacheOptions):

+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.Configuration.get -> string?
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.Configuration.set -> void
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ConfigurationOptions.get -> StackExchange.Redis.ConfigurationOptions?
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ConfigurationOptions.set -> void
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ConnectionMultiplexerFactory.get -> System.Func<System.Threading.Tasks.Task<StackExchange.Redis.IConnectionMultiplexer!>!>?
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ConnectionMultiplexerFactory.set -> void
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.InstanceName.get -> string?
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.InstanceName.set -> void
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ProfilingSession.get -> System.Func<StackExchange.Redis.Profiling.ProfilingSession!>?
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.ProfilingSession.set -> void
+ Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.RedisOutputCacheOptions.RedisOutputCacheOptions() -> void

which is the new API to register SE.Redis into output cache

Alternatives considered

1. eating the GC overhead, zero API change - hugely undesirable in this context
2. have a new GetAsync API that returns some kind of "data with lifetime" - however, there's no good metaphor for this, and it pushes the buffer management into each individual buffer implementation, rather than having one buffer implementation and having the cache implementations just worry about cache

[ghost]

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.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

This second interface is optional / separate because not all types will wish to opt in; in particular, in the case of the existing in-proc implementation, it will be preferable to just keep returning the existing byte[]; as such, the output cache middleware will test for this API and use the most appropriate.

Why is GetAsync different than SetAsync in this regard? Would a default implementation that calls the byte[]-returning GetAsync and copies it to the destination writer be that bad?

[mitchdenny]

I'm not super enthusiastic about the GetAsync on IOutputCacheBufferWriterStore returning a ValueTask<bool> where the implementation on IOutputCacheStore returns ValueTask<byte[]?>. I get what you are trying to do though.

I'm wondering whether it would be better to put two new Get and Set methods on the new interface and leave the original one alone.

[mgravell]

@mitchdenny to make it explicit; the bool there is the equivalent of return null; in the byte[] case, to signify misses - something we can't express otherwise. I'm cautious of treating zero bytes as a miss, because I know full well that some serialization formats have zero bytes as a valid payload.

Re putting them both on the same interface: probably fine here. Any implementation that is perfectly happy to return byte[] - probably also happy to take one. That would mean renaming the interface though, and naming is hard :)

Maybe IOutputCacheBufferStore? Open to suggestions here.

[mgravell]

@mitchdenny TryGetAsync?

+ public interface IOutputCacheBufferStore : IOutputCacheStore
+ {
+     ValueTask<bool> TryGetAsync(string key, IBufferWriter<byte> destination, CancellationToken cancellationToken);
+     ValueTask SetAsync(string key, ReadOnlySequence<byte> value, ReadOnlyMemory<string> tags, TimeSpan validFor, CancellationToken cancellationToken);
+ }

[halter73]

API Review Notes:

• The issue is that the byte[] buffers are suboptimal for out-of-process performance.
• Might we need backpressure for the IBufferWriter<byte> destination?
  • The default output cache limit 64 MB per response and 100 MB total
  • A PipeWriter would allow us to do smaller writes and chunk it up awaiting backpressure.
• On the flip side, would we ever start writing to the cache before all the data is buffered in memory?
  • If we did, we could pass the PipeReader for the request body, but we want to validate the entire body before we start caching anyway.
• And async TryGet is unusual. Are there any other examples of this in the framework? There's no out, but it seems to align.
• Does StackExchangeRedisCacheServiceCollectionExtensions make sense for AddStackExchangeRedisOutputCache?
  • Yes. This is the place AddStackExchangeRedisCache which adds IDistributedCache lives.
• Does it make sense for IOutputCacheBufferStore to derive from IOutputCacheStore?
  • Yes. We want the EvictByTagAsync method, and it's easy to implement the byte[] methods.
  • We do want real implementations for the IOutputCacheStore byte[] methods and not something that just throws then.
• Do we like the IOutputCacheBufferStore?
  • There's no IBufferWriter anymore but the pipe still gives you buffers.
  • We hear no better proposals.

API Approved!

// Microsoft.AspNetCore.OutputCaching.dll
namespace Microsoft.AspNetCore.OutputCaching;

+ public interface IOutputCacheBufferStore : IOutputCacheStore
+ {
+     ValueTask<bool> TryGetAsync(string key, PipeWriter destination, CancellationToken cancellationToken);
+     ValueTask SetAsync(string key, ReadOnlySequence<byte> value, ReadOnlyMemory<string> tags, TimeSpan validFor, CancellationToken cancellationToken);
+ }

// Microsoft.Extensions.Caching.StackExchangeRedis.dll
namespace Microsoft.Extensions.Caching.StackExchangeRedis;

public static class StackExchangeRedisCacheServiceCollectionExtensions
{
+     public static IServiceCollection AddStackExchangeRedisOutputCache(
+         this IServiceCollection services, Action<RedisCacheOptions> setupAction)
}

[adityamandaleeka]

@mgravell Can this be closed now?

[mgravell]

@adityamandaleeka I honestly don't know :) last time I closed an API one down, it was too soon and caused problems - maybe @halter73 can tell me the right time to close them?

[halter73]

We can definitely close this now that all the changes are in, but it helps to leave a new comment to indicate what has changed since June 20th when this API was approved. Editing comments is not ideal for when people come back and want to see how we arrived at our API decisions.

Here's what @mgravell wrote in the API review email thread about why #50457 which added the new Microsoft.AspNetCore.OutputCaching.StackExchangeRedis NuGet package was necessary:

Previously an API change was approved to add asp.net output cache support via SE.Redis, by extending the existing Microsoft.Extensions.Caching.StackExchangeRedis package; this was implemented, but... bad things. This inadvertently added a framework reference to aspnetcore into an OOB package that previously had no such framework reference (and that could be / was actively used without aspnetcore).

This undesirable outcome is the state in RC1. It is proposed to resolve this, by reverting​​ the changes to the pre-existing package, and instead to add a new Microsoft.AspNetCore.OutputCaching.StackExchangeRedis OOB package. This change has been updated (with strike-out) in the existing API proposal:

RC2 API Update

// Microsoft.Extensions.Caching.StackExchangeRedis.dll
namespace Microsoft.Extensions.Caching.StackExchangeRedis;

public static class StackExchangeRedisCacheServiceCollectionExtensions
{
-     public static IServiceCollection AddStackExchangeRedisOutputCache(
-         this IServiceCollection services, Action<RedisCacheOptions> setupAction)
}

+ // Microsoft.AspNetCore.OutputCaching.StackExchangeRedis.dll (NuGet net8.0 only)

+ namespace Microsoft.AspNetCore.OutputCaching.StackExchangeRedis;

+ public sealed class RedisOutputCacheOptions
+ {
+    public RedisOutputCacheOptions();
+
+    public string? Configuration { get; set; }
+    public ConfigurationOptions? ConfigurationOptions { get; set; }
+    public Func<Task<IConnectionMultiplexer>>? ConnectionMultiplexerFactory { get; set; }
+    public string? InstanceName { get; set; }
+    public Func<ProfilingSession>? ProfilingSession { get; set; }
+ }

namespace Microsoft.Extensions.DependencyInjection;

+ public static class StackExchangeRedisOutputCacheServiceCollectionExtensions
+ {
+    public static IServiceCollection AddStackExchangeRedisOutputCache(this IServiceCollection services, Action<RedisOutputCacheOptions> setupAction);
+ }

Note: All the public properties on the new RedisOutputCacheOptions type are copied from Microsoft.Extensions.Caching.StackExchangeRedis.RedisOutputCacheOptions. The only other difference beside name and namespace is that it's sealed and does not implement IOptions<TSelf>.

As for why we didn't just use the existing options type from the StackExchangeRedis package, see the discussion at #50457 (comment).

tl;dr: there was some discussion about independently configuring a redis-back IDistributedCache and IOutputCache, but the main thing seems to be we didn't want to add a Microsoft.Extensions.Caching.StackExchangeRedis dependency to the new Microsoft.AspNetCore.OutputCaching.StackExchangeRedis package for just the options type.