x/time/rate: document the behavior of negative n

[dsnet]

The methods AllowN, ReserveN, WaitN take in a user-specified number of tokens to take. The documentation does not specify what happens when the number of tokens is negative.

Without digging into the details, it appears that you can actually specify a negative n, in which case it returns that number of tokens to the rate limiter.

[andig]

I'm using this mechanism for managing potential operations: get token, check if operation does actually execute or errors, in case of error return token. It would be helpful to establish that this is the desired behavior.

[dsnet]

I was planning on using it for something similar:

• I'm unable to trust the client on how much work something will cost, so I pessimistically grab the worst case cost.
• I perform the operation for the client.
• I credit back whatever was unused.

[cherrymui]

What is the expected behavior for negative n? Does the implementation have the expected behavior, so just needs to be documented? Feel free to send a CL. Thanks.

cc @ianlancetaylor

[dsnet]

I would expect the behavior to be equivalent to returning N tokens back to the bucket. If doing so exceeds the burst size, then it clamps the number tokens to the bucket size. It might make sense to add an explicit Credit and CreditN method that performs this operation in a more obvious way.

[dsnet]

@andig, out of curiosity, which method were you using to return tokens back to the limiter: Allow, Reserve, or Wait?

[andig]

AllowN, found by experiment and looking at the sources:

// rate limit
if !v.limiter.Allow() {
    // log.Printf("#%.3d soc rate limit (%s)", v.id, claims.Subject)
    return nil, ErrRateLimitExceeded
}

soc, err := v.vehicle.SoC()

res := pb.SoCReply{
    Soc: soc,
}

// increase rate limit
if err != nil {
    if errors.Is(err, api.ErrMustRetry) {
        v.limiter.AllowN(time.Now(), -1)
    } else {
        log.Printf("#%.3d soc %v (%s)", v.id, err, claims.Subject)
    }
}

[juanma9613]

Hi @andig is there any progress on this? Can I keep using this behavior?

[iamcalledrob]

I think it's worth mentioning that the current implementation of Wait doesn't work as you might expect when returning tokens to the bucket.

Example: https://go.dev/play/p/Nkmjt24P3uR

// Rate limit, 1 event per 10 seconds, no burst.
l := rate.NewLimiter(0.1, 1)

// After 1s, return a token to the bucket.
// You might expect this to unblock Wait, but it doesn't.
go func() {
    <-time.After(1 * time.Second)
    l.AllowN(time.Now(), -1)
    log.Println("Token returned to bucket")
}()

// Observe: wait is not unblocked when a token is added.
for i := 0; ; i++ {
    _ = l.Wait(context.Background())
    log.Printf("Allowed #%d!", i)
}

Results in the following. Note how a token being returned doesn't unblock Wait.

2009/11/10 23:00:00 Allowed #0!
2009/11/10 23:00:01 Token returned to bucket
2009/11/10 23:00:10 Allowed #1!
2009/11/10 23:00:10 Allowed #2!

I think this would need to be documented, and potentially the behaviour of Wait changed to account for this. Wait would need to unblock when a token is returned to the bucket.

At a glance, this should be doable without spawning additional goroutines, and could be done in a backwards-compatible way if the "return to bucket" capability was via a new method.

[andig]

AllowN, found by experiment and looking at the sources:

I did not validate though if that would unblock any waiting routines as I've always used Allow() first to check for rate limit.