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: Timer.Stop documentation incorrect for Timer returned by AfterFunc #17600

Closed
iangudger opened this issue Oct 26, 2016 · 8 comments
Closed

time: Timer.Stop documentation incorrect for Timer returned by AfterFunc #17600

iangudger opened this issue Oct 26, 2016 · 8 comments

Comments

@iangudger
Copy link
Contributor

@iangudger iangudger commented Oct 26, 2016

The time.Timer.Stop() documentation states "To prevent the timer firing after a call to Stop, check the return value and drain the channel." This is incorrect because time.AfterFunc does not set time.Timer.C. nil channels always block, so if you follow the documentation, your code will block indefinitely.

Is there a way to be sure that a timer created with time.AfterFunc has fired if it is going to fire? Either way, this should be added to the documentation.

@bradfitz bradfitz added this to the Go1.8 milestone Oct 26, 2016
@ianlancetaylor ianlancetaylor changed the title time: Timer.Stop() documentation incorrect time: Timer.Stop documentation incorrect for Timer returned by AfterFunc Oct 27, 2016
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 27, 2016

I think that for a Timer returned by AfterFunc timer.Stop will return true if the function has not been called (in which case the function will not be called) and returns false if the function has already been called. I think this is just a documentation issue.

@iangudger
Copy link
Contributor Author

@iangudger iangudger commented Oct 27, 2016

I tested it and that is not always true. There seems to be a race.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 27, 2016

Interesting. I don't doubt you, but, looking at the code, I don't see how that is possible. Can you provide a test case?

@iangudger
Copy link
Contributor Author

@iangudger iangudger commented Oct 27, 2016

On my workstation the following reliably panics:

package main

import (
    "time"
)

func main() {
    ch := make(chan struct{})
    var timer *time.Timer
    for i := 0; i < 1000000; i++ {
        if timer != nil && !timer.Stop() {
            ch = make(chan struct{})
        }
        timer = time.AfterFunc(0, func() {
            close(ch)
        })
    }
}

If you reduce the number of iterations to a small number, say 100, it rarely panics.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 27, 2016

I think there is a data race in that code. I think timer.Stop reliably returns false if the function has been called, but that does not mean that the function has completed.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 27, 2016

There is definitely a data race:

==================
WARNING: DATA RACE
Read at 0x00c42002a008 by goroutine 6:
  main.main.func1()
      /home/bradfitz/race.go:15 +0x49

Previous write at 0x00c42002a008 by main goroutine:
  main.main()   
      /home/bradfitz/race.go:12 +0x1d4

Goroutine 6 (running) created at:
  time.goFunc()
      /home/bradfitz/go/src/time/sleep.go:164 +0x77
==================
panic: close of closed channel

goroutine 37 [running]:
panic(0x495160, 0xc42014c000)
        /home/bradfitz/go/src/runtime/panic.go:531 +0x1ad
main.main.func1()
        /home/bradfitz/race.go:15 +0x5b
created by time.goFunc
        /home/bradfitz/go/src/time/sleep.go:164 +0x78
exit status 2
@iangudger
Copy link
Contributor Author

@iangudger iangudger commented Oct 27, 2016

Yes, there is a data race. I am suggesting that we update the documentation because I don't think it is clear. I will propose some changes.

@quentinmit quentinmit added the NeedsFix label Nov 7, 2016
@gopherbot
Copy link

@gopherbot gopherbot commented Nov 11, 2016

CL https://golang.org/cl/33131 mentions this issue.

@gopherbot gopherbot closed this in fabb411 Nov 11, 2016
@golang golang locked and limited conversation to collaborators Nov 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.