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

time: Ticker.Reset(0) deadlocks #49315

Closed
tfg13 opened this issue Nov 3, 2021 · 6 comments
Closed

time: Ticker.Reset(0) deadlocks #49315

tfg13 opened this issue Nov 3, 2021 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.

Comments

@tfg13
Copy link

tfg13 commented Nov 3, 2021

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

$ go version
go version go1.17.2 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/XXXX/Library/Caches/go-build"
GOENV="/Users/XXXX/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/XXXX/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/XXXX/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.17.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/ct/1_807xnx3yvbbym9xwypyc540000gn/T/go-build1490192283=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/weOQpA3YuFD

What did you expect to see?

Either:

  • panic the Reset call for setting duration to zero (this will panic in time.NewTicker, but does not in time.Reset)
  • tick the channel as fast as possible (delay zero), lots of "tick" output in my example

What did you see instead?

The ticker fires exactly one more tick, then stops - the example deadlocks because nothing is running anymore.
For any Duration >0 the ticker does not stop ticking after one tick.
@seankhliao seankhliao changed the title ticker.Reset(0) has unexpected behavior (ticks exactly one more time) time: Ticker.Reset(0) deadlocks Nov 3, 2021
@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 3, 2021
@seankhliao
Copy link
Member

cc @rsc

@gopherbot
Copy link

Change https://golang.org/cl/361074 mentions this issue: time: make Ticker.Reset(0) panic

@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2021

I suspect panicking will be too invasive of a change. I'm sure there's plenty of code in the wild that does some math to compute a duration for Reset and isn't careful about whether it's <= 0 when what the caller would instead want is "right away".

@zhouguangyuan0718
Copy link
Contributor

I suspect panicking will be too invasive of a change. I'm sure there's plenty of code in the wild that does some math to compute a duration for Reset and isn't careful about whether it's <= 0 when what the caller would instead want is "right away".

But if we make it tick as fast as possible when period is 0. This change is invasive, too.
Is it a reasonable choice if we keep as it is, and make this behavior as a feature?

@bradfitz
Copy link
Contributor

bradfitz commented Nov 4, 2021

Oh, sorry, I read this too quickly about being about Timer.Reset.

@zhouguangyuan0718
Copy link
Contributor

I sent the CL above to fix this bug by adding a check. If duration <= 0, it will panic.
https://go-review.googlesource.com/c/go/+/361074/

If it is the right way to fix it, could anyone review it continue and submit?
Thanks.

lstoll added a commit to lstoll/tjts that referenced this issue Mar 5, 2022
@lstoll
Fix fetching deadlock when connection issues occur
ecbf507 
When there's network issues (thanks vodafone), chunks can fail to fetch. When this
happens our calculation to reset the timer based on fetched data returns 0, which
deadlocks the ticker (golang/go#49315). As this will panic
in future releases as well, reset the ticker to 5 seconds when this occurs. This should
stop hammering things when network issues occur too. Exponential backoff would be nice,
but y'know.

Also upgrade to go 1.18rc while here, because why not.
lstoll added a commit to lstoll/tjts that referenced this issue Mar 5, 2022
@lstoll
Fix fetching deadlock when connection issues occur
0ac438a 
When there's network issues (thanks vodafone), chunks can fail to fetch. When this
happens our calculation to reset the timer based on fetched data returns 0, which
deadlocks the ticker (golang/go#49315). As this will panic
in future releases as well, reset the ticker to 5 seconds when this occurs. This should
stop hammering things when network issues occur too. Exponential backoff would be nice,
but y'know.

Also upgrade to go 1.18rc while here, because why not.

Fixes #5
@lstoll lstoll mentioned this issue Mar 5, 2022
Merged
@mash mash mentioned this issue Apr 4, 2022
Merged
@golang golang locked and limited conversation to collaborators Nov 5, 2022
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
No milestone
Development

No branches or pull requests
5 participants
@bradfitz @tfg13 @gopherbot @seankhliao @zhouguangyuan0718