Navigation Menu

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

Jump to bottom

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

Closed
bradbl opened this issue Jul 1, 2020 · 7 comments
Closed

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

bradbl opened this issue Jul 1, 2020 · 7 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@bradbl
Copy link

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

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 6, 2020
@crazymi
Copy link

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 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 commented May 28, 2021 •
edited

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

@nishanths
Copy link

The line number in @akolar's comments are out of date, so here's fixed links (using a constant commit).

Old: https://github.com/golang/time/blob/master/rate/rate.go#L391
Fixed: https://github.com/golang/time/blob/ed9ce3a/rate/rate.go#L388

	return time.Nanosecond * time.Duration(1e9*seconds)

Old: https://github.com/golang/time/blob/master/rate/rate.go#L335
Fixed: https://github.com/golang/time/blob/ed9ce3a/rate/rate.go#L332

	ok := n <= lim.burst && waitDuration <= maxFutureReserve

@nishanths nishanths mentioned this issue Jul 18, 2021
Closed
@ianwoolf
Copy link
Contributor

ianwoolf commented Sep 17, 2021 •
edited

i want get someone's help to review my change about this issue: https://go-review.googlesource.com/c/time/+/323429

please feel free to review it

@gopherbot
Copy link

Change https://golang.org/cl/323429 mentions this issue: x/time/rate: func advance and durationFromTokens bug when limit is zero

@replay replay mentioned this issue Jan 14, 2022
Merged
@golang golang locked and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
8 participants
@cagedmantis @bcmills @nishanths @akolar @gopherbot @crazymi @ianwoolf @bradbl