[API Proposal]: Add leaveOpen concept to PartitionedRateLimiter.TranslateKey

[BrennanConroy]

Background and motivation

Originally proposed and approved in #65400 (comment), the PartitionedRateLimiter<TResource>.TranslateKey(...) method wraps and returns a new instance of PartitionedRateLimiter. Currently we are assuming no ownership of the wrapped limiter to be on the safe side. The proposal is to add a flag to give ownership to the wrapper so Dispose and DisposeAsync will cleanup the wrapped limiter.

A few examples of prior-art for this type of API:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

Line 181 in dc2db86

public SslStream(Stream innerStream, bool leaveInnerStreamOpen)

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Line 93 in dc2db86

public StreamWriter(Stream stream, Encoding? encoding = null, int bufferSize = -1, bool leaveOpen = false)

runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs

Line 100 in b12e288

public virtual Stream AsStream(bool leaveOpen = false)

API Proposal

namespace System.Threading.RateLimiting;

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter);
+    public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter, bool leaveOpen = false);
}

API Usage

var limiter = PartitionedRateLimiter.Create<string, int>(resource =>
{
    if (resource == "1")
    {
        return RateLimitPartition.GetConcurrencyLimiter(1,
            _ => new ConcurrencyLimiterOptions(1, QueueProcessingOrder.NewestFirst, 1));
    }
    return RateLimitPartition.GetNoLimiter(2);
});

var translateLimiter = limiter.TranslateKey<int>(i => i.ToString(), leaveOpen: false);
translateLimiter.Acquire(1);

translateLimiter.Dispose();

Alternative Designs

Leave as-is and require user to keep track of the inner limiter and call Dispose/DisposeAsync on that when done.

Risks

If more options are added for TranslateKey then we'll end up with additional overloads. Like how StreamWriter and SslStream have a bunch of overloads.

I am unable to think of options we might want to add in the future though.

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

Originally proposed and approved in #65400 (comment), the PartitionedRateLimiter<TResource>.TranslateKey(...) method wraps and returns a new instance of PartitionedRateLimiter. Currently we are assuming no ownership of the wrapped limiter to be on the safe side. The proposal is to add a flag to give ownership to the wrapper so Dispose and DisposeAsync will cleanup the inner limiter.

A few examples of prior-art for this type of API:

runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.cs

Line 181 in dc2db86

public SslStream(Stream innerStream, bool leaveInnerStreamOpen)

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Line 93 in dc2db86

public StreamWriter(Stream stream, Encoding? encoding = null, int bufferSize = -1, bool leaveOpen = false)

runtime/src/libraries/System.IO.Pipelines/src/System/IO/Pipelines/PipeReader.cs

Line 100 in b12e288

public virtual Stream AsStream(bool leaveOpen = false)

API Proposal

namespace System.Threading.RateLimiting;

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter);
+    public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter, bool leaveOpen = false);
}

API Usage

var limiter = PartitionedRateLimiter.Create<string, int>(resource =>
{
    if (resource == "1")
    {
        return RateLimitPartition.GetConcurrencyLimiter(1,
            _ => new ConcurrencyLimiterOptions(1, QueueProcessingOrder.NewestFirst, 1));
    }
    return RateLimitPartition.GetNoLimiter(2);
});

var translateLimiter = limiter.TranslateKey<int>(i => i.ToString(), leaveOpen: false);
translateLimiter.Acquire(1);

translateLimiter.Dispose();

Alternative Designs

Leave as-is and require user to keep track of the inner limiter and call Dispose/DisposeAsync on that when done.

Risks

If more options are added for TranslateKey then we'll end up with additional overloads. Like how StreamWriter and SslStream have a bunch of overloads.

I am unable to think of options we might want to add in the future though.

Author:	BrennanConroy
Assignees:	-
Labels:	api-suggestion, area-System.Threading, blocking
Milestone:	-

[terrajobst]

Video

• Let's make the parameter explicit to ensure people understand the transfer of ownership
  • Let's also rename it to WithTranslatedKey

namespace System.Threading.RateLimiting;

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public PartitionedRateLimiter<TOuter> TranslateKey<TOuter>(Func<TOuter, TResource> keyAdapter);
+    public PartitionedRateLimiter<TOuter> WithTranslatedKey<TOuter>(Func<TOuter, TResource> keyAdapter, bool leaveOpen);
}