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

x/sync/singleflight: occasional failures in TestForget #42092

Closed
bcmills opened this issue Oct 20, 2020 · 1 comment
Closed

x/sync/singleflight: occasional failures in TestForget #42092

bcmills opened this issue Oct 20, 2020 · 1 comment
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Oct 20, 2020 

--- FAIL: TestForget (0.00s)
    singleflight_test.go:160: Third call should not be started because was started during second execution
    singleflight_test.go:163: We should receive result produced by second call, expected: 2, got 3
FAIL
FAIL	golang.org/x/sync/singleflight	61.958s

The failing test is the regression test added for #31420 (CC @cuonglm @bradfitz).

The bug appears to be in the test:
https://github.com/golang/sync/blob/b3e1573b75205f2905c4b27986413d2e2be9803c/singleflight/singleflight_test.go#L153-L157

In that section, nothing guarantees that the second invocation is still running when g.Do is evaluated. (The second goroutine can complete immediately after close(secondCh), causing the third invocation to be evaluated.)

2020-10-08T14:14:35-b3e1573/android-386-emu
2019-09-11T18:51:00-cd5d95a/freebsd-amd64-race
2019-04-23T02:48:10-1122301/freebsd-amd64-race
2019-04-23T02:48:10-1122301/windows-amd64-race
@gopherbot gopherbot added this to the Unreleased milestone Oct 20, 2020
@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. labels Oct 20, 2020
@bcmills bcmills modified the milestones: Unreleased, Backlog Oct 20, 2020
@gopherbot
Copy link

Change https://golang.org/cl/263877 mentions this issue: singleflight: fix TestForget

sherifabdlnaby pushed a commit to sherifabdlnaby/sync that referenced this issue Feb 24, 2021
@cuonglm @sherifabdlnaby
singleflight: fix flaky TestForget
2efa197 
There can be a race condition in current TestForget, that said when
"close(secondCh)" is executed, the second goroutine will be finished
immediately, causing the third "g.Do" is evaluated.

To fix this, we change to use "g.DoChan" for both second and third. In
second, we block to make sure it's still running at the time we call
third. after then we unblock second and verify the result.

Fixes golang/go#42092

Change-Id: I980fdf109a531e2b7a74c8149b4fcaa338775e08
Reviewed-on: https://go-review.googlesource.com/c/sync/+/263877
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Oct 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
2 participants
@bcmills @gopherbot