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: add lock for getting burst #37357

Closed
fatedier opened this issue Feb 21, 2020 · 5 comments
Closed

x/time/rate: add lock for getting burst #37357

fatedier opened this issue Feb 21, 2020 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@fatedier
Copy link

What version of Go are you using (go version)?

$ go version
go1.13.5

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

// Burst returns the maximum burst size. Burst is the maximum number of tokens
// that can be consumed in a single call to Allow, Reserve, or Wait, so higher
// Burst values allow more events to happen at once.
// A zero Burst allows no events, unless limit == Inf.
func (lim *Limiter) Burst() int {
	return lim.burst
}

// SetBurst is shorthand for SetBurstAt(time.Now(), newBurst).
func (lim *Limiter) SetBurst(newBurst int) {
	lim.SetBurstAt(time.Now(), newBurst)
}

// SetBurstAt sets a new burst size for the limiter.
func (lim *Limiter) SetBurstAt(now time.Time, newBurst int) {
	lim.mu.Lock()
	defer lim.mu.Unlock()

	now, _, tokens := lim.advance(now)

	lim.last = now
	lim.tokens = tokens
	lim.burst = newBurst
}

Burst can be modified in func (lim *Limiter) SetBurst(newBurst int), so it may be protected in func (lim *Limiter) Burst() int.

What did you expect to see?

I send PR to golang/time#9

What did you see instead?

@gopherbot gopherbot added this to the Unreleased milestone Feb 21, 2020
@toothrot toothrot added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 21, 2020
@toothrot
Copy link
Contributor

/cc @Sajmani

@fatedier
Copy link
Author

Ping @Sajmani

@Sajmani
Copy link
Contributor

Sajmani commented Jun 25, 2020

Yes, it looks like lim.Burst should hold lim.mu. Key thing to check is that this doesn't introduce a deadlock, that is, we need to be certain that lim.Burst() is not called while lim.mu is held. A quick audit of rate.go looks OK.

We also need to fix the log line at 232:
https://github.com/golang/time/blob/master/rate/rate.go#L232

	if n > burst && limit != Inf {
		return fmt.Errorf("rate: Wait(n=%d) exceeds limiter's burst %d", n, lim.burst)
	}

This should use burst, not lim.burst, since it's outside the locked section.

@fatedier
Copy link
Author

@Sajmani OK. I have update my PR to fix this.

@Sajmani
Copy link
Contributor

Sajmani commented Jun 29, 2020 via email

@golang golang locked and limited conversation to collaborators Jul 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants