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

Comments

@Petelin
Copy link

@Petelin Petelin commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 15, 2017

What is error access number?

@Petelin

This comment has been minimized.

Copy link
Author

@Petelin Petelin commented Dec 15, 2017

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Author

@Petelin 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.

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