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: NewLimiter with burst of 1 and rate of 0 always allows #39984

Open
bradbl opened this issue Jul 1, 2020 · 4 comments
Open

x/time/rate: NewLimiter with burst of 1 and rate of 0 always allows #39984

bradbl opened this issue Jul 1, 2020 · 4 comments

Comments

@bradbl
Copy link

@bradbl bradbl commented Jul 1, 2020

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

$ go version
go version go1.14.2 darwin/amd64

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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bradb/Library/Caches/go-build"
GOENV="/Users/bradb/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/bradb/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/pw/bgnb7cj52n31mkr7q0843l8r0000gn/T/go-build081265086=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

See https://play.golang.org/p/u6Hygbausg7

r := rate.NewLimiter(0, 1)
fmt.Println(r.Allow()) // Expect true
fmt.Println(r.Allow()) // Expect false

What did you expect to see?

I expected the first call to r.Allow() to return true and the second call to return false.

I expect this based on the following docs:

A Limiter controls how frequently events are allowed to happen. It implements a "token bucket" of size b, initially full and refilled at rate r tokens per second.

and

Limiter has three main methods, Allow, Reserve, and Wait. Most callers should use Wait. Each of the three methods consumes a single token.

So NewLimiter should create a full bucket of size 1 that never refills and each call to Allow should consume one token from the bucket.

What did you see instead?

Instead it appears that calls to Allow never consume a token and always return true.

@gopherbot gopherbot added this to the Unreleased milestone Jul 1, 2020
@akolar
Copy link

@akolar akolar commented Jul 2, 2020

I think what happens is that time.Duration in https://github.com/golang/time/blob/master/rate/rate.go#L391 overflows (casting math.Inf(1) to int64): https://play.golang.org/p/KlTGAsvjrvQ

As the time needed to refill the bucket is, in this particular case, negative, this then evaluates to true: https://github.com/golang/time/blob/master/rate/rate.go#L335

@crazymi
Copy link

@crazymi crazymi commented Jul 31, 2020

@bradbl IMO, r.Allow() in your playground should return false for both lines. You may find out this description in doc A zero Limit allows no events..
@akolar Thanks for the advice, and I sent the PR! Feel free to check it!

@bradbl
Copy link
Author

@bradbl bradbl commented Feb 10, 2021

@bradbl IMO, r.Allow() in your playground should return false for both lines. You may find out this description in doc A zero Limit allows no events..

This isn't a zero valued Limiter.

@ianwoolf
Copy link
Contributor

@ianwoolf ianwoolf commented May 28, 2021

I agree that the first call to function r.Allow() should return true. Because zero limit only means that new tokens are not allowed, but burst tokens should be allowed.

I sent a PR, please feel free to check it.
https://go-review.googlesource.com/c/time/+/323429

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants