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

Comments

Projects
None yet
5 participants
@ianlancetaylor
Copy link
Contributor

commented Sep 13, 2017

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

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 13, 2017

@dsnet

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

CL coming up in a few minutes.

@gopherbot

This comment has been minimized.

Copy link

commented Sep 15, 2017

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

@mdcnz

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@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

This comment has been minimized.

Copy link

commented Sep 15, 2017

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

@gopherbot gopherbot closed this in a72e26f Sep 15, 2017

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