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 incorrect usage of time.Timer #45839

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

tigrato
Copy link
Contributor

@tigrato tigrato commented Aug 26, 2024

Invoking timer.Stop on a timer that has already fired causes Stop to return false, and any subsequent drain operation will block because the channel remains open.

This PR addresses the blocking issue by wrapping the drain operation in a non-blocking select.

@tigrato tigrato added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 labels Aug 26, 2024
@github-actions github-actions bot requested review from kiosion and zmb3 August 26, 2024 15:14
Calling `timer.Stop` in a timer that already fired results in `Stop` returning false and subsequent drain will block because the channel isn't closed.

This PR fixes the drain behaviour by wrapping the drain operation in a non-blocking select.
@tigrato tigrato force-pushed the tigrato/fix-test-debug-reconc branch from b35843f to aec8ede Compare August 26, 2024 15:15
@zmb3 zmb3 changed the title fix incorrect usage of timer.Timer fix incorrect usage of time.Timer Aug 26, 2024
@zmb3
Copy link
Collaborator

zmb3 commented Aug 26, 2024

Does this change still hold for Go 1.23, or do we need to leave a note to go back and update?

For a chan-based timer created with NewTimer(d), as of Go 1.23, any receive from t.C after Stop has returned is guaranteed to block rather than receive a stale time value from before the Stop; if the program has not received from t.C already and the timer is running, Stop is guaranteed to return true. Before Go 1.23, the only safe way to use Stop was insert an extra <-t.C if Stop returned false to drain a potential stale value. See the NewTimer documentation for more details.

@tigrato
Copy link
Contributor Author

tigrato commented Aug 26, 2024

Does this change still hold for Go 1.23, or do we need to leave a note to go back and update?

For a chan-based timer created with NewTimer(d), as of Go 1.23, any receive from t.C after Stop has returned is guaranteed to block rather than receive a stale time value from before the Stop; if the program has not received from t.C already and the timer is running, Stop is guaranteed to return true. Before Go 1.23, the only safe way to use Stop was insert an extra <-t.C if Stop returned false to drain a potential stale value. See the NewTimer documentation for more details.

Unfortunately, Go 1.23 won't fix this situation directly check.
It fixes the deadlock because we no longer need to drain the channel every time we stop the timer, but for existing code, this problem would persist.

Although it doesn't fix the problem, Go 1.23 removed the misleading example where I got this incorrect behavior. check the discussion golang/go#27169

@tigrato tigrato enabled auto-merge August 26, 2024 15:33
@tigrato tigrato added this pull request to the merge queue Aug 26, 2024
Merged via the queue into master with commit f5c3fdb Aug 26, 2024
40 checks passed
@tigrato tigrato deleted the tigrato/fix-test-debug-reconc branch August 26, 2024 16:32
@public-teleport-github-review-bot

@tigrato See the table below for backport results.

Branch Result
branch/v15 Failed
branch/v16 Create PR

@public-teleport-github-review-bot

@tigrato See the table below for backport results.

Branch Result
branch/v15 Create PR
branch/v16 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 backport/branch/v16 no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants