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

fix: stop timer in a way that works before and after go1.23 #579

Closed
wants to merge 1 commit into from

Conversation

gammazero
Copy link
Contributor

In this case, a stopped timer is created. Since the timer duration is set to 1 hour and is immediately stopped, the timer should never fire and the channel should never need to be cleared (as is generally needed for go1.22). For go-1.23 the timer semantics have changed so that reading the timer channel following Stop will always block. Therefore, for both these cases, only calling Stop is sufficient to create the stopped timer.

In this case, a stopped timer is created. Since the timer duration is set to 1 hour and is immediately stopped, the timer shoud never fire and the channel should never need to be cleared (as is generally needed for go1.22). For go-1.23 the timer semantics have changed so that reading the timer channle following Stop will always block. Therefore, for both these casses, only calling Stop is sufficient to crete the stopped timer.
@gammazero gammazero requested a review from vyzo September 9, 2024 15:53
Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, I think t.Stop() might always return true in which case we don't need to do anything.

Otherwise, we will have to also add a go 1.23 reuirement to go.md

@vyzo
Copy link
Collaborator

vyzo commented Sep 9, 2024

By not doing anything, it means drop the patch altogether.

Can you please test the behavior of timer.Stop() in go1.23? If it always returns true, then we can close this pr.

@gammazero
Copy link
Contributor Author

In go1.23 timer.Stop, following time.NewTimer always returns true unless:

  • Stop has been called
  • The timer channel has been read

Once Stop has been called, another call to Stop always returns false. Since the timer channel is unbuffered, there is no chance for a value to have been written to it.

@vyzo
Copy link
Collaborator

vyzo commented Sep 9, 2024

ok, please add a requirement for go1.23 to go.mod then.

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code is fine. Let's wait for the upstream fix: golang/go#69312

Alternatively, you can change the time.Hour to math.MaxInt64 and keep the rest of the code same.

@gammazero
Copy link
Contributor Author

Let's just leave it for now. Changing it to require go 1.23 may cause problems with things that depend on this package.

@gammazero gammazero closed this Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants