[API Proposal]: Rename RateLimiter.WaitAsync

[BrennanConroy]

Background and motivation

WaitAsync doesn't really convey that it acquires a lease. It more looks like it will wait until more permits are available. Sort of like ChannelReader.WaitToReadAsync.

We think changing the name to AcquireAsync is clearer that it might need to wait for more permits to become available and then acquire a lease. Similar to ChannelReader.ReadAsync.

API Proposal

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}

API Usage

RateLimiter limiter = GetLimiter();
var lease = limiter.Acquire(1);
if (!lease.IsAcquired)
{
    lease = await limiter.AcquireAsync(1);
}

Alternative Designs

No response

Risks

No response

[ghost]

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

Issue Details

---

Background and motivation

WaitAsync doesn't really convey that it acquires a lease. It more looks like it will wait until more permits are available. Sort of like ChannelReader.WaitToReadAsync.

We think changing the name to AcquireAsync is clearer that it might need to wait for more permits to become available and then acquire a lease. Similar to ChannelReader.ReadAsync.

API Proposal

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}

API Usage

RateLimiter limiter = GetLimiter();
var lease = limiter.Acquire(1);
if (!lease.IsAcquired)
{
    lease = await limiter.AcquireAsync(1);
}

Alternative Designs

No response

Risks

No response

Author:	BrennanConroy
Assignees:	-
Labels:	area-System.Threading, blocking, api-ready-for-review
Milestone:	-

[terrajobst]

Video

• We don't like the proposed name AcquireAsync as it would imply that the existing sync Acquire method is the non-async counterpart, which some tooling also depends on (e.g. code fixer in async context).
  • We decided on WaitAndAcquireAsync

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> WaitAndAcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public ValueTask<RateLimitLease> WaitAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> WaitAndAcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}

[halter73]

We've had some more conversation about this on the ASP.NET side, and we just can't swallow the wordy WaitAndAcquireAsync name. We have a new proposal that will hopefully address the concerns about "the proposed name AcquireAsync as it would imply that the existing sync Acquire method is the non-async counterpart, which some tooling also depends on (e.g. code fixer in async context)." and that WaitAsync might not imply anything was acquired. Since this is just another rename of the exact same API, I'm reopening this to keep context.

The basic idea is to rename WaitAndAcquireAsync to AcquireAsync and rename Acquire to TryAcquire to make it clear that it isn't just the blocking version of AcquireAsync. It is weird to have a Try method that doesn't return a bool, but we think that's less bad than shipping a method named WaitAndAcquireAsync.

API Proposal

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire( int permitCount = 1);
+    public RateLimitLease TryAcquire(int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(TResource resource, int permitCount);
+    protected abstract RateLimitLease TryAcquireCore(TResource resource, int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire(TResource resource, int permitCount = 1);
+    public RateLimitLease TryAcquire(TResource resource, int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(TResource resource, int permitCount);
+    protected abstract RateLimitLease TryAcquireCore(TResource resource, int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}

API Usage

RateLimiter limiter = GetLimiter();
var lease = limiter.TryAcquire(1);
if (!lease.IsAcquired)
{
    lease = await limiter.AcquireAsync(1);
    // ...
}

[stephentoub]

rename Acquire to TryAcquire to make it clear that it isn't just the blocking version of AcquireAsync. It is weird to have a Try method that doesn't return a bool, but we think that's less bad than shipping a method named WaitAndAcquireAsync

Why not make it actually follow the Try pattern and return a bool with the lease as the out parameter? If it's unsuccessful, do you actually need the resulting lease object? The bool value would be duplicated in the object, but it would always be true in this case.

we just can't swallow the wordy WaitAndAcquireAsync

Who can't? Why?

It is the accurate description, and having a TryAquire and Aquire just changes the problem. Having Try not follow the pattern of returning bool is also hard to swallow in that it's confusing for consumers and weakens the pattern overall, and it suggests that if Aquire returns successfully then the lease has been successfully acquired, since it lacks the corresponding Try, e.g. TryDequeue returns false on failure while Dequeue throws on failure.

[BrennanConroy]

If it's unsuccessful, do you actually need the resulting lease object?

Yes, the lease may contain metadata, e.g. an estimate for how long you should wait until you call TryAcquire again. Token Bucket, Fixed Window, and Sliding Window can all come up with estimates for a retry delay.

One potential rename could be to keep Acquire and name the other method QueueAcquireAsync. This name suggests that it has different behavior from Acquire, which it does.

[stephentoub]

QueueAcquireAsync is so much better than
WaitAndAcquireAsync?

[halter73]

I don't like QueueAcquireAsync either. My primary goal with renaming this is to make the async API which provides backpressure more usable by making it less of a mouthful. I think this is the API most people will want to use over Acquire, but Acquire's short and simple name almost makes it seem like its preferred.

The name Acquire also does not communicate that it will not be as effective as acquiring permits as the async version. I understand not wanting to breaking long established conventions around the prefix Try in a method name, but I really want the async API to be called AcquireAsync and give the sync API the longer name indicating it doesn't wait in line.

How about AcquireIfImmediatelyAvailable and AcquireAsync?

[stephentoub]

How about AcquireIfImmediatelyAvailable and AcquireAsync?

That's not horrible, but talk about a mouthful :) Or... AttemptAcquire and AcquireAsync?

Alternatively, did we consider actually just making Acquire behave like AcquireAsync, except via synchronous rather than asynchronous blocking? It could have an int-based overload for a timeout, e.g. Acquire(0) would be identical in behavior to what Acquire() is today. In this regard it would end up being like the Wait/WaitAsync pairs we have on some sync primitives.

[halter73]

That's not horrible, but talk about a mouthful :) Or... AttemptAcquire and AcquireAsync?

I like it.

Alternatively, did we consider actually just making Acquire behave like AcquireAsync, except via synchronous rather than asynchronous blocking? It could have an int-based overload for a timeout, e.g. Acquire(0) would be identical in behavior to what Acquire() is today. In this regard it would end up being like the Wait/WaitAsync pairs we have on some sync primitives.

I really like this idea. The problem I see with it is that then we'd want to still provide a non-acquired RateLimitLease with whatever metadata is expected (e.g. RETRY_AFTER and REASON_PHRASE) rather than throwing like we would in the async version if you trip the CancellationToken.

We could probably do this, but then that creates an unfortunate scenario where the blocking API provides better behavior than the non-blocking API at least in terms of providing metadata after a timeout. We could add a timeout in addition to the CancellationToken to the async API but that would be really weird. We could instead return a non-acquired lease rather than throwing when the CancellationToken trips in AcquireAsync, but that would also really weird.

I don't want to rule out the timout change, and I think we should discuss that further, but the AttemptAcquire and AcquireAsync rename seems like an easy win. I don't want that to be lost in pursuit of something better that may not happen, so I'm going to propose that rename for tomorrow's API review.

API Proposal

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire(int permitCount = 1);
+    public RateLimitLease AttemptAcquire(int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(int permitCount);
+    protected abstract RateLimitLease AttemptAcquireCore(int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire(TResource resource, int permitCount = 1);
+    public RateLimitLease AttemptAcquire(TResource resource, int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(TResource resource, int permitCount);
+    protected abstract RateLimitLease AttemptAcquireCore(TResource resource, int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}

API Usage

RateLimiter limiter = GetLimiter();
var lease = limiter.TryAcquire(1);
if (!lease.IsAcquired)
{
    lease = await limiter.AcquireAsync(1);
    // ...
}

[terrajobst]

• Looks as proposed

namespace System.Threading.RateLimiting;

public abstract class RateLimiter : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire(int permitCount = 1);
+    public RateLimitLease AttemptAcquire(int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(int permitCount);
+    protected abstract RateLimitLease AttemptAcquireCore(int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(int permitCount, CancellationToken cancellationToken);
}

public abstract class PartitionedRateLimiter<TResource> : IAsyncDisposable, IDisposable
{
-    public RateLimitLease Acquire(TResource resource, int permitCount = 1);
+    public RateLimitLease AttemptAcquire(TResource resource, int permitCount = 1);

-    protected abstract RateLimitLease AcquireCore(TResource resource, int permitCount);
+    protected abstract RateLimitLease AttemptAcquireCore(TResource resource, int permitCount);

-    public ValueTask<RateLimitLease> WaitAndAcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);
+    public ValueTask<RateLimitLease> AcquireAsync(TResource resourceID, int permitCount = 1, CancellationToken cancellationToken = default);

-    protected abstract ValueTask<RateLimitLease> WaitAndAcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
+    protected abstract ValueTask<RateLimitLease> AcquireAsyncCore(TResource resourceID, int permitCount, CancellationToken cancellationToken);
}