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: reservation's delay is longer than expected if the previous one is not ok #52584

Open
cncal opened this issue Apr 27, 2022 · 3 comments · May be fixed by golang/time#22
Open

x/time/rate: reservation's delay is longer than expected if the previous one is not ok #52584

cncal opened this issue Apr 27, 2022 · 3 comments · May be fixed by golang/time#22
Labels
NeedsInvestigation
Milestone

Comments

@cncal
Copy link
Contributor

@cncal cncal commented Apr 27, 2022

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

$ go version
go version go1.17 darwin/amd64

Does this issue reproduce with the latest release?

Yes, it reproduces with go1.18.

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/cn/Library/Caches/go-build"
GOENV="/Users/cn/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/cn/Workspace/golang/pkg/mod"
GOOS="darwin"
GOPATH="/Users/cn/Workspace/golang"
GOPRIVATE=""
GOPROXY="https://goproxy.cn,direct"
GOROOT="/Users/cn/Workspace/gosdk/go1.17"
GOSUMDB="sum.golang.google.cn"
GOTMPDIR=""
GOTOOLDIR="/Users/cn/Workspace/gosdk/go1.17/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/9d/p9d0dmmj04zfrll8cm7rndsm0000gn/T/go-build2220765180=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://go.dev/play/p/vGnnjsqOcTp?v=goprev.

What did you expect to see?

I expect r.Delay() returns 200ms, just like https://go.dev/play/p/knb4-IPMUFd?v=goprev.

What did you see instead?

r.Delay() returns 300ms. Both of previous reservations are not ok, why the later reservation returns different delay.

@gopherbot gopherbot added this to the Unreleased milestone Apr 27, 2022
@cagedmantis cagedmantis added the NeedsInvestigation label May 2, 2022
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented May 2, 2022

/cc @bcmills @Sajmani

ZekeLu added a commit to ZekeLu/time that referenced this issue May 13, 2022
… failed

In the following cases, the reserveN is a no-op, and the state of
the limiter should not be changed:
1. n exceeds the Limiter's burst size
2. maxFutureReserve is less than the waitDuration

Fixes golang/go#52584
ZekeLu added a commit to ZekeLu/time that referenced this issue May 13, 2022
…s failed

In the following cases, the reserveN is a no-op, and the state of
the limiter should not be changed:
1. n exceeds the Limiter's burst size
2. maxFutureReserve is less than the waitDuration

Fixes golang/go#52584
@gopherbot
Copy link

@gopherbot gopherbot commented May 13, 2022

Change https://go.dev/cl/406154 mentions this issue: rate: the state of the limiter should not be changed when the requests failed

@ZekeLu
Copy link

@ZekeLu ZekeLu commented May 13, 2022

I think the correct value is 300ms.

See https://go.dev/play/p/X_jeZbv_mRy?v=goprev which removes the line lim.ReserveN(t0, 3) that causes the error.

Note that the burst is 2 so that at t1 before the request (lim.ReserveN(t1, 2).OK()), there are 2 tokens in the bucket (instead of 3).

Regarding the error caused by lim.ReserveN(t0, 3), I have sent a CL to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants