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 example easily leads to deadlocks #27169

Open
palsivertsen opened this Issue Aug 23, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@palsivertsen
Copy link

commented Aug 23, 2018

I needed timeout functionality for one of my projects, so I looked in the time package. My timeouts where fallbacks in case a channel receive took too long. Most of the time the channel would receive before the timeout and I wanted to release the timeout resources when they where no longer needed. Documentation for time.After() says:

[...] If efficiency is a concern, use NewTimer instead and call Timer.Stop if the timer is no longer needed.

So I used a time.Timer and according to the documentation for time.Timer.Stop() one should drain the channel if time.Timer.Stop() returns false:

if !t.Stop() {
	<-t.C
}

I later discovered that my threads got stuck on receive like in this playground example when timer where triggered before I called stop:

t := time.NewTimer(time.Second * 3)
defer func() {
	if !t.Stop() {
		<-t.C
	}
}()
<-t.C

Wrapping the drain in a select seems to do the trick:

t := time.NewTimer(time.Second * 3)
defer func() {
	t.Stop()
	select {
	case <-t.C:
	default:
	}
}()
<-t.C

Documentation should make it clear how to safely drain the channel.

@FMNSSun

This comment has been minimized.

Copy link

commented Aug 23, 2018

TL/DR: This is incorrect usage and the documentation kinda mentions it but it takes a while to understand it correctly so while it's documented it could be documented better.

For example, assuming the program has not received from t.C already

t := time.NewTimer(time.Second * 3)
defer func() {
	if !t.Stop() {
		<-t.C
	}
}()
<-t.C

Isn't this incorrect usage because you've already received from t.C. Isn't the point of the timer to fire after the delay except if you call Stop on it? <-t.C already waits for the timer to fire so the stop in the defered function is entirely useless because the timer has already fired anyway?

The way it actually works is that Stop() returns false in case the timer has already fired which means UNLESS you haven't ALREADY read from it then there's a value in t.C you might want to read. Obviously this doesn't work if you've already read from t.C. Stop will return false regardless (as the timer has already fired) but you've already read from t.C earlier thus you deadlock on <-t.C.

t := time.NewTimer(time.Second * 3)
defer func() {
	t.Stop()
	select {
	case <-t.C:
	default:
	}
}()
<-t.C

This prevents the deadlock, sure and it's always safe to do that because if Stop() returns true you enter the default case and if it returns false you enter the default case as well because t.C is empty because you've already read it but since you enter the default case anyway in this example you might as well just remove the whole select. Still, this isn't the intended usage of Stop().

@FMNSSun

This comment has been minimized.

Copy link

commented Aug 23, 2018

FWIW: This would be an example of proper usage:

package main

import "time"
import "fmt"

func main() {
	t := time.NewTimer(time.Second * 3)
	foo := make(chan int)
	go func() { foo <- 1 }()
	select {
	case <-t.C:
		fmt.Println("timeout")
	case <-foo:
		fmt.Println("foo")
		if !t.Stop() {
			<-t.C
		}
	}
}

@artyom

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

My timeouts where fallbacks in case a channel receive took too long. Most of the time the channel would receive before the timeout and I wanted to release the timeout resources when they where no longer needed.

I believe you don't really need to drain the timer channel for this, calling Timer.Stop will suffice:

timer := time.NewTimer(3 * time.Second)
defer timer.Stop()
select {
case res := <- workChannel:
    return res, nil
case <-timer.C:
    return nil, ErrTimeout
}

You may find such pattern in use in standard library.

@bradfitz bradfitz changed the title time/Timer.Stop documentation example easily leads to deadlocks time: Timer.Stop documentation example easily leads to deadlocks Aug 23, 2018

@palsivertsen

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

@FMNSSun
Thanks for the explanation. Your example looks somewhat like what I did in the first place. But I had more channels in my select and didn't like all the extra t.Stop() calls:

t := time.NewTimer(time.Second * 3)
bar := make(chan int)
chicken := make(chan int)
egg := make(chan int)
go func() { foo <- 1 }()
select {
case <-t.C:
	fmt.Println("timeout")
case <-foo:
	fmt.Println("foo")
	if !t.Stop() {
		<-t.C
	}
case <-chicken:
	fmt.Println("chicken")
	if !t.Stop() {
		<-t.C
	}
case <-egg:
	fmt.Println("egg")
	if !t.Stop() {
		<-t.C
	}
}

@artyom

I believe you don't really need to drain the timer channel for this, calling Timer.Stop will suffice

Looks scary. What if timer triggers between the select block and the defer? Won't you have a thread stuck on channel send?

@FMNSSun

This comment has been minimized.

Copy link

commented Aug 24, 2018

@palsivertsen No because timer uses a buffered channel with capacity 1 exactly for this reason: that it can't get stuck if there's nobody reading from it.

Also... it might make sense in that case to move the t.Stop past the select instead of repeating it in every case.

@palsivertsen

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

timer uses a buffered channel with capacity 1 exactly for this reason: that it can't get stuck if there's nobody reading from it.

Cool. I did not know that.

Also... it might make sense in that case to move the t.Stop past the select instead of repeating it in every case.

Wouldn't that deadlock if case <-t.C: happens?

@FMNSSun

This comment has been minimized.

Copy link

commented Aug 24, 2018

@palsivertsen it would but you could set a flag in the case <-t.C case and then only invoke stop if that flag isn't already set. But probably matter of personal taste.

@ran-eh

This comment has been minimized.

Copy link

commented Mar 20, 2019

+1 this issue just cost me an hour :( I also have the keep-alive scenario. Perhaps the the docs should link to this discussion?

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Mar 20, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I don't think we want to link to this discussion.

Does anyone have specific improvements to suggest? Anyone want to send a pull request? Thanks.

@palsivertsen

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

Some suggestions/thoughts:

  • Update the doc to underline that ignoring the value of the time.Timer.C channel is safe because the channel has a buffer length of one.
  • Remove channel draining from time.Timer.Stop documentation. Are there any usecases where one would need/want to drain the channel when stopping (not resetting) the timer?
  • Make time.Timer.Reset do the draining internally, thus no need to expose the drain concept in the docs. This sadly changes the behaviour of time.Timer.Reset, possibly breaking existing code. Adding time.Timer.ResetAndDrain might be an alternative.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.