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

runtime: TestFutexSleep can permanently disable async preemption until process exit. #50749

Closed
prattmic opened this issue Jan 21, 2022 · 1 comment
Assignees
Labels
NeedsFix release-blocker
Milestone

Comments

@prattmic
Copy link
Member

@prattmic prattmic commented Jan 21, 2022

TestFutexSleep calls runtime.Futexsleep, which temporarily disables asynchronous preemption. Unfortunately, TestFutexSleep calls this from multiple goroutines, creating a race condition that may result in asynchronous preemption remaining disabled for the remainder of the process lifetime.

This was a contributing factor in #45867, where async preemption would have allowed forward progress (though #45867 still has a different work conservation bug).

This test was created in https://go.dev/cl/7876043 as a regression test for buggy division logic in futexsleep. It was made largely obsolete (IMO) about 6 months later by https://go.dev/cl/11575044, which moved this logic to runtime.timediv, which could be tested independently.

This test has a bad track record of flakiness bugs based on the file history, thus I propose removing this test entirely in favor of a new unit test for timediv.

cc @aclements @mknyszek @cherrymui

@prattmic prattmic added the NeedsFix label Jan 21, 2022
@prattmic prattmic added this to the Go1.18 milestone Jan 21, 2022
@prattmic prattmic self-assigned this Jan 21, 2022
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 21, 2022

Change https://golang.org/cl/380058 mentions this issue: runtime: replace TestFutexsleep with TestTimediv

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
TestFutexsleep was originally created in CL 7876043 as a
regression test for buggy division logic in futexsleep. Several months
later CL 11575044 moved this logic to timediv (called by futexsleep).

This test calls runtime.Futexsleep, which temporarily disables
asynchronous preemption. Unfortunately, TestFutexSleep calls this from
multiple goroutines, creating a race condition that may result in
asynchronous preemption remaining disabled for the remainder of the
process lifetime.

We could fix this by moving the async preemption disable to the main
test function, however this test has had a history of flakiness. As an
alternative, this CL replaces the test wholesale with a new test for
timediv, covering the overflow logic without the difficulty of dealing
with futex.

Fixes golang#50749.

Change-Id: If9e1dac63ef1535adb49f9a9ffcaff99b9135895
Reviewed-on: https://go-review.googlesource.com/c/go/+/380058
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
@prattmic prattmic self-assigned this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix release-blocker
Projects
None yet
Development

No branches or pull requests

2 participants