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: Zero limiter allows events #18763

Open
AudriusButkevicius opened this issue Jan 23, 2017 · 11 comments
Open

x/time/rate: Zero limiter allows events #18763

AudriusButkevicius opened this issue Jan 23, 2017 · 11 comments

Comments

@AudriusButkevicius
Copy link
Contributor

@AudriusButkevicius AudriusButkevicius commented Jan 23, 2017

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

Audrius@Audrius foo $ go version
go version go1.8rc2 windows/amd64

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

Audrius@Audrius foo $ go env
set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Gohome
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\cygwin64\tmp\go-build503767043=/tmp/go-build -gno-record-gcc-switches
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

Run the following on amd64:

package main

import (
        "golang.org/x/time/rate"
        "fmt"
)

func main() {
        r := rate.NewLimiter(0, 100)
        if r.Allow() {
                fmt.Println("broken")
        } else {
                fmt.Println("ok")
        }
}

What did you expect to see?

Output ok

What did you see instead?

Output broken, yet documentation says:

The zero value is a valid Limiter, but it will reject all events.

Please note this does work as documented on arm64 which I suspect is a compiler bug either in everything else apart from arm64 or a bug in arm64 and this library.
See #18762,

@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented Jan 23, 2017

What's the behavior of this on Go1.7?

@AudriusButkevicius

This comment has been minimized.

Copy link
Contributor Author

@AudriusButkevicius AudriusButkevicius commented Jan 23, 2017

Given the issue cropped up on our android app, which is built using 1.7.x, I'd say the same.

@vcabbage

This comment has been minimized.

Copy link
Member

@vcabbage vcabbage commented Jan 23, 2017

rate.NewLimiter(0, 100) isn't creating a zero valued Limiter (the burst is non-zero) and the documentation states that the token bucket is initially full:

It implements a "token bucket" of size b, initially full and refilled at rate r tokens per second.

var r rate.Limiter and r := rate.NewLimiter(0, 0) print "ok" using the example program.

Seems like the behavior is consistent with the documentation.

@AudriusButkevicius

This comment has been minimized.

Copy link
Contributor Author

@AudriusButkevicius AudriusButkevicius commented Jan 23, 2017

In that case, the generic behaviour is not consistent ccompared to arm64, which goes back to the original compiler issue I've opened.

@calmh

This comment has been minimized.

Copy link
Contributor

@calmh calmh commented Jan 23, 2017

The real quote is on the Limit type, which says a zero rate allows no events. (On mobile, so not pasting)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 23, 2017

The docs say:

The zero value is a valid Limiter, but it will reject all events. Use NewLimiter to create non-zero Limiters.

NewLimiter always returns a non-zero limiter, regardless of its arguments.

Let's close this bug but we can keep #18762 open for the floating point arm/arm64 issue.

@bradfitz bradfitz closed this Jan 23, 2017
@vcabbage

This comment has been minimized.

Copy link
Member

@vcabbage vcabbage commented Jan 23, 2017

@bradfitz There does seem to be an inconsistency since (or at least ambiguity), as @calmh pointed out, https://godoc.org/golang.org/x/time/rate#Limit says A zero Limit allows no events.

@calmh

This comment has been minimized.

Copy link
Contributor

@calmh calmh commented Jan 24, 2017

(I wrote a comment about how this should not have been closed, but the corresponding issue now lives in #18762 instead so this is fine. I'll go away now.)

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 24, 2017

Okay, reopening. I didn't see there were two parts of the docs talking about zero values.

@bradfitz bradfitz reopened this Jan 24, 2017
@madhuravi

This comment has been minimized.

Copy link

@madhuravi madhuravi commented Jan 26, 2018

@bradfitz Here is the doc talking about zero limiter https://github.com/golang/time/blob/master/rate/rate.go#L39

I am running into the same issue. If my limit is 0 (burst size is 1),
rateLimiter.Reserve() actually returns OK=true and delay=0s which seems incorrect.

I want my burst size to be non zero because I want threads to block on Wait() and not return error.

@yulrizka

This comment has been minimized.

Copy link

@yulrizka yulrizka commented Mar 21, 2019

like @madhuravi mentioned, the behavior is surprising to me that Wait on limit 0 burst 1 is not giving error or waiting

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
You can’t perform that action at this time.