Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dsnet opened this issue Jul 27, 2022 · 9 comments
Open

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

dsnet opened this issue Jul 27, 2022 · 9 comments
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Jul 27, 2022

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.

@gopherbot gopherbot added this to the Unreleased milestone Jul 27, 2022
@andig
Copy link
Contributor

andig commented Jul 27, 2022

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
Copy link
Member Author

dsnet commented Jul 27, 2022

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
Copy link
Member

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

@cherrymui cherrymui added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 27, 2022
@dsnet
Copy link
Member Author

dsnet commented Jul 27, 2022

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
Copy link
Member Author

dsnet commented Jul 27, 2022

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

@andig
Copy link
Contributor

andig commented Jul 27, 2022

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
Copy link

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

@iamcalledrob
Copy link

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
Copy link
Contributor

andig commented Dec 30, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants