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 not covering all the cases #35627

Closed
ikalyvas opened this issue Nov 15, 2019 · 5 comments
Closed

time: Timer.Stop documentation not covering all the cases #35627

ikalyvas opened this issue Nov 15, 2019 · 5 comments

Comments

@ikalyvas
Copy link

@ikalyvas ikalyvas commented Nov 15, 2019

Hello,
In the section where the Stop method of a timer object is described there is a snippet about ensuring to drain the channel in case this method returns False.
However, it does not mention explicitly that this code snippet works only when the Stop returns False in case the timer has already expired. Because only in this case the channel read will not block.
If the timer is already stopped , this operation will block indefinitely, therefore will not drain the channel since no value will be ever sent.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 15, 2019

Do you have a specific change to suggest?

In particular, note that the documentation for that example already says "assuming the program has not received from t.C already".

@josharian josharian changed the title Timer.Stop documentation not covering all the cases time: Timer.Stop documentation not covering all the cases Nov 17, 2019
@ikalyvas
Copy link
Author

@ikalyvas ikalyvas commented Nov 19, 2019

Yes, but the clause "assuming the program has not received from t.C already" is not making any difference without any additional info about the event that causes the t.Stop() to return False.

So, assuming the program has not received from t.C already and the timer has already expired, then try to drain the channel.
Because otherwise, some other part of the program(a go routine maybe) could have already tried to stop the timer, so at the moment the snippet does the check even it has not yet received from the t.C, it does not know if the t.Stop()==False is due to expiration or previously stopped action.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 19, 2019

Can you show us the complete docs for Stop that you think would be clearer? Thanks.

@ikalyvas
Copy link
Author

@ikalyvas ikalyvas commented Nov 19, 2019

So it would go like this:

func (t *Timer) Stop() bool

Stop prevents the Timer from firing. It returns true if the call stops the timer, false if the timer has already expired or been stopped. Stop does not close the channel, to prevent a read from the channel succeeding incorrectly.

To ensure the channel is empty after a call to Stop, check the return value and drain the channel. For example,
a) assuming the program has not received from t.C already
b) we have not stopped the timer yet
c) and the timer has been expired :

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

Otherwise, if somewhere else in the program the timer has been already stopped, trying to drain the channel is going to block.

This cannot be done concurrent to other receives from the Timer's channel.

For a timer created with AfterFunc(d, f), if t.Stop returns false, then the timer has already expired and the function f has been started in its own goroutine; Stop does not wait for f to complete before returning. If the caller needs to know whether f is completed, it must coordinate with f explicitly.

However, on a closer look i start to believe that returning the same value (False) in two different scenarios would not be such a good idea.Because the programmer might need to do multiple checks and be sure that the timer is not already stopped before draining this channel.
Perhaps it could be better the t.Stop() return False only in the case where the timer is expired and panic when is already stopped, making the decision easier rather than worrying if some forgotten go routine has already called Stop on this channel.Anyway, i think such implementation detail would require other things so perhaps the change to documentation is easier to be done.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 20, 2019

Change https://golang.org/cl/207849 mentions this issue: time: further clarifications to the (*Timer).Stop docs

@gopherbot gopherbot closed this in 001fe7f Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.