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.

Sign up for GitHub

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

Jump to bottom

runtime,time: timer.Stop returns false even when no value is read from the channel [1.23 backport] #69333

Closed
gopherbot opened this issue Sep 7, 2024 · 4 comments
Closed

runtime,time: timer.Stop returns false even when no value is read from the channel [1.23 backport] #69333

gopherbot opened this issue Sep 7, 2024 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.23.2

Comments

@gopherbot
Copy link
Contributor

@ianlancetaylor requested issue #69312 to be considered for backport to the next 1.23 minor release.

@gopherbot Please open a backport to 1.23.

This bug makes it difficult or impossible to write timer code that uses Stop and Reset and works correctly for all versions of Go.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Sep 7, 2024
@gopherbot gopherbot mentioned this issue Sep 7, 2024
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 7, 2024
@gopherbot gopherbot added this to the Go1.23.2 milestone Sep 7, 2024
@cagedmantis cagedmantis added the CherryPickApproved Used during the release process for point releases label Sep 18, 2024
@cagedmantis
Copy link
Contributor

This is approved as it is a bug without a workaround.

@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Sep 18, 2024
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/616096 mentions this issue: [release-branch.go1.23] runtime: if stop/reset races with running timer, return correct result

@lidel lidel mentioned this issue Sep 27, 2024
Closed
gopherbot pushed a commit that referenced this issue Sep 27, 2024
@ianlancetaylor @gopherbot
[release-branch.go1.23] runtime: if stop/reset races with running tim…
3b2e846 
…er, return correct result

The timer code is careful to ensure that if stop/reset is called
while a timer is being run, we cancel the run. However, the code
failed to ensure that in that case stop/reset returned true,
meaning that the timer had been stopped. In the racing case
stop/reset could see that t.when had been set to zero,
and return false, even though the timer had not and never would fire.

Fix this by tracking whether a timer run is in progress,
and using that to reliably detect that the run was cancelled,
meaning that stop/reset should return true.

For #69312
Fixes #69333

Change-Id: I78e870063eb96650638f12c056e32c931417c84a
Reviewed-on: https://go-review.googlesource.com/c/go/+/611496
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
(cherry picked from commit 2ebaff4)
Reviewed-on: https://go-review.googlesource.com/c/go/+/616096
Reviewed-by: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link
Contributor Author

Closed by merging CL 616096 (commit 3b2e846) to release-branch.go1.23.

@mknyszek mknyszek changed the title runtime: timer.Stop returns false even when no value is read from the channel [1.23 backport] runtime,time: timer.Stop returns false even when no value is read from the channel [1.23 backport] Oct 1, 2024
@cristaloleg cristaloleg mentioned this issue Oct 2, 2024
Merged
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/617596 mentions this issue: [release-branch.go1.23] runtime: clear isSending bit earlier

@codeboten codeboten mentioned this issue Oct 3, 2024
Closed
@rootulp rootulp mentioned this issue Oct 4, 2024
Merged
gopherbot pushed a commit that referenced this issue Oct 8, 2024
@ianlancetaylor @gopherbot
[release-branch.go1.23] runtime: clear isSending bit earlier
cc16cdf 
I've done some more testing of the new isSending field.
I'm not able to get more than 2 bits set. That said,
with this change it's significantly less likely to have even
2 bits set. The idea here is to clear the bit before possibly
locking the channel we are sending the value on, thus avoiding
some delay and some serialization.

For #69312
For #69333

Change-Id: I8b5f167f162bbcbcbf7ea47305967f349b62b0f4
Reviewed-on: https://go-review.googlesource.com/c/go/+/617596
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Go1.23.2
Development

No branches or pull requests
2 participants
@cagedmantis @gopherbot