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

time: constructing your own time.Ticker used to work but now fails #21874

Closed
ianlancetaylor opened this issue Sep 13, 2017 · 7 comments
Closed

Comments

@ianlancetaylor
Copy link
Contributor

People sometimes build their own time.Ticker values in order to inject a time.Ticker value into some other function. This lets them control when values are sent on the channel, in order to write tests that work more quickly.

Whether we think this is a good strategy or not, it did work with 1.9. It fails on current tip. Sample program:

package main

import "time"

func main() {
	c := make(chan time.Time)
	t := &time.Ticker{C: c}
	t.Stop()
}

Running this program crashes:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x43c2ee]

goroutine 1 [running]:
time.stopTimer(0xc420062008, 0xc420062000)
	/home/iant/go/src/runtime/time.go:117 +0x2b
time.(*Ticker).Stop(0xc420062000)
	/home/iant/go/src/time/tick.go:46 +0x31
main.main()
	foo.go:8 +0x74
exit status 2

It should be possible to fix this to avoid breaking people's programs unnecessarily.

CC @valyala

@dsnet
Copy link
Member

dsnet commented Sep 13, 2017

We should also document whether constructing your own timer manually is supported. In my own code, I have always done:

t := time.NewTimer(0)
<-t.C

since I was not sure whether manual construction was supported or not.

@odeke-em
Copy link
Member

Alright, I can confirm that this CL https://go-review.googlesource.com/34784 aka commit 76f4fd8 at 76f4fd8#diff-67606dc98d3216ed00b49367a70885ecL128 introduced this bug.
Particularly the runtime code for delTimer, we previously found the timer to stop from the global slice of running timers, but now we are trying to delete a timer from a timersBucket of the provided ticker, yet invoking &time.Ticker{C: c} skips the setup in which runtime.startTimer creates and sets the appropriate *timersBucket to set in the runtimeTimer for each new Ticker.

/cc @valyala

@odeke-em
Copy link
Member

CL coming up in a few minutes.

@gopherbot
Copy link

Change https://golang.org/cl/63970 mentions this issue: runtime, time: lazily assign a runtimeBucket before Stop

@mdcnz
Copy link

mdcnz commented Sep 15, 2017

@odeke-em, with all due respect &time.Ticker{C: c} creates a ticker with a zero value timer, and the timer can't be started, correct? When Stop() is called, would it not be better to do nothing? In other words, return early if the timer is stopped, rather than lazily starting the timer only in order to stop it?

@odeke-em
Copy link
Member

@mdcnz that's polite of you. No worries, it's a great question, and that's the purpose of healthy discourse, thank you for the comment!

In deed, it creates a zero value timer, and like yours, @ianlancetaylor also suggested the same on the review. My rationale for following the entire addtimer routine was to preserve an almost similar code path to the former code(since that's what it did before), and ensure that the runtimeBucket indexed by the goroutine's ID was being cleaned up. However, I'll be updating the CL to follow yours and @ianlancetaylor same suggestion.

@mdcnz
Copy link

mdcnz commented Sep 15, 2017

Thanks for your response @odeke-em, nice one.

@golang golang locked and limited conversation to collaborators Sep 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants