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
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@iangudger
Copy link
Contributor

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

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

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

@ianlancetaylor
Copy link
Contributor

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

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

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

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

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 The path to resolution is known, but the work has not been done. label Nov 7, 2016
@gopherbot
Copy link

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

@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.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants