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: question or bug report about Limiter.advance #23145

Open
Petelin opened this issue Dec 15, 2017 · 5 comments
Open

x/time/rate: question or bug report about Limiter.advance #23145

Petelin opened this issue Dec 15, 2017 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@Petelin
Copy link

Petelin commented Dec 15, 2017 •
edited

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

go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/xiaolin.zhang/gopath"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/wd/vt73dmmj5y38hft8kg5l1wgm0000gq/T/go-build260497711=/tmp/go-build -gno-record-gcc-switches -fno-common"
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"

What did you do?

i create a limit=rate.NewLimiter(400, 400), run 810 goroutine call wait at the sametime,
error access number should >= 10. however, less then 10.

After read source code

func (lim *Limiter) advance(now time.Time) (newNow time.Time, newLast time.Time, newTokens float64) {
	last := lim.last
	//if now.Before(last) {
	//	last = now
	//}
I think it is because, some time now is before last, when case happens, just set last back???
that cause some wait should timeout, but let it go.

so, why we need set back last?
@gopherbot gopherbot added this to the Unreleased milestone Dec 15, 2017
@bradfitz bradfitz changed the title x/time wrong x/time/rate: question or bug report about Limiter.advance Dec 15, 2017
@bradfitz
Copy link
Contributor

What is error access number?

@Petelin
Copy link
Author

Petelin commented Dec 15, 2017 •
edited 

package main

import (
	"context"
	"sync"
	"sync/atomic"
	"time"

	"log"

	"golang.org/x/time/rate"
)

var limiter = rate.NewLimiter(1000, 40)

func do() (err error) {
	ctx, cancle := context.WithTimeout(context.Background(), time.Second*3)
	defer cancle()
	return limiter.Wait(ctx)
}
func main() {
	const overcount = 10000
	var accessFailCount int32 = 0
	totalCount := int(limiter.Limit()) + limiter.Burst() + overcount
	wg := new(sync.WaitGroup)
	wg.Add(totalCount)
	for i := 0; i < totalCount; i++ {
		go func() {
			defer wg.Done()
			err := do()
			if err != nil {
				//FIXME: return fmt.Errorf("rate: Wait(n=%d) would exceed context deadline", n)
				//i think this is a bad code format, error should always return with a `type`.
				atomic.AddInt32(&accessFailCount, 1)
			}
		}()
	}
	wg.Wait()
	log.Println(accessFailCount >= overcount, accessFailCount, overcount)
}

import : i set ctx timeout to 3s. all goroutine can be done without any doubt.

so after 3s, there should have 1000 + 10000 + 40 - 40 - 3 * 1000 = 8000

but result is 7000+

@as
Copy link
Contributor

as commented Dec 15, 2017

The program posted above assumes too many things, it contains data races. Don't append to the happenTimes slice concurrently. I would avoid atomic altogether.

@Petelin
Copy link
Author

Petelin commented Dec 15, 2017

i do assume something, but i cannot write a more strain forward one.

i think all task is done in 1s. and happenTimes does not really matter(just for reference)

@Petelin
Copy link
Author

Petelin commented Dec 15, 2017

Because of limited clock resolution, at high rates, the actual rate may be up to 1% different from the specified rate.

i find this in other realize document.

but i still think

	//if now.Before(last) {
	//	last = now
	//}

is wired. since you callback clock.

@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 3, 2019
@b97tsk b97tsk mentioned this issue Mar 26, 2021
Closed
@seankhliao seankhliao mentioned this issue Feb 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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
5 participants
@bradfitz @ALTree @as @gopherbot @Petelin