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.

Sign up for GitHub

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

Closed
cncal opened this issue Apr 27, 2022 · 3 comments
Closed

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@cncal
Copy link
Contributor

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 Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 2, 2022
@cagedmantis
Copy link
Contributor

/cc @bcmills @Sajmani

ZekeLu added a commit to ZekeLu/time that referenced this issue May 13, 2022
@ZekeLu
rate: the state of the limiter should not be changed when the rquests…
75d04f0 
… 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
@ZekeLu
rate: the state of the limiter should not be changed when the request…
d54a5ce 
…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
@ZekeLu ZekeLu mentioned this issue May 13, 2022
Closed
@gopherbot
Copy link

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
Contributor

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.

@ZekeLu ZekeLu mentioned this issue Jun 10, 2022
Closed
ZekeLu added a commit to ZekeLu/time that referenced this issue Oct 17, 2022
@ZekeLu
rate: the state of the limiter should not be changed when the request…
127e7a4 
…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
@ZekeLu ZekeLu mentioned this issue Oct 17, 2022
Closed
@tentree tentree mentioned this issue Oct 18, 2022
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 16, 2022
@krajorama krajorama mentioned this issue Jan 11, 2023
Merged
3 tasks
@golang golang locked and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

rate: the state of the limiter should not be changed when the requests failed ZekeLu/time
5 participants
@cagedmantis @ZekeLu @dmitshur @gopherbot @cncal