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

Jump to bottom

runtime/race: deflake tests #14119

Open
dvyukov opened this issue Jan 27, 2016 · 8 comments
Open

runtime/race: deflake tests #14119

dvyukov opened this issue Jan 27, 2016 · 8 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@dvyukov
Copy link
Member

dvyukov commented Jan 27, 2016

See https://go-review.googlesource.com/#/c/18968 for context.
Race tests run with GOMAXPROCS=1, this makes them more or less reliable. But the ultimate solution is to explicitly annotate tests with required execution order by means of a special "invisible" synchronization primitive (that's what is done for C++ ThreadSanitizer tests). But that would require going over 350 tests.
@dvyukov dvyukov self-assigned this Jan 27, 2016
@gopherbot
Copy link

CL https://golang.org/cl/18968 mentions this issue.

@aclements aclements added this to the Go1.7 milestone Jan 27, 2016
gopherbot pushed a commit that referenced this issue Jan 27, 2016
@dvyukov @rsc
runtime/race: run tests with GOMAXPROCS=1
572f766 
We set GOMAXPROCS=1 to prevent test flakiness.
There are two sources of flakiness:
1. Some tests rely on particular execution order.
   If the order is different, race does not happen at all.
2. Ironically, ThreadSanitizer runtime contains a logical race condition
   that can lead to false negatives if racy accesses happen literally at the same time.
Tests used to work reliably in the good old days of GOMAXPROCS=1.
So let's set it for now. A more reliable solution is to explicitly annotate tests
with required execution order by means of a special "invisible" synchronization primitive
(that's what is done for C++ ThreadSanitizer tests). This is issue #14119.

This reduces flakes on RaceAsFunc3 test from 60/3000 to 1/3000.

Fixes #14086
Fixes #14079
Fixes #14035

Change-Id: Ibaec6b2b21e27b62563bffbb28473a854722cf41
Reviewed-on: https://go-review.googlesource.com/18968
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 29, 2016
@rsc
Copy link
Contributor

rsc commented Sep 29, 2016

Ideally this would happen, but I think Dmitriy is the only one who can really do it.

@rsc rsc modified the milestones: Unplanned, Go1.8 Sep 29, 2016
This was referenced Mar 18, 2017
Closed
Closed
@bradfitz bradfitz modified the milestones: Go1.9, Unplanned Mar 18, 2017
@bradfitz
Copy link
Contributor

#19604 was marked as a dup of this bug.

If these tests can't be made reliable, I think it's time to disable them. Or give them, say, 10 chances to succeed before reporting a failure.

Marking this as Go 1.9 to either fix them, disable them or make them try harder to pass.

@josharian
Copy link
Contributor

Another instance of #19604: https://build.golang.org/log/72ead1aab99537284ab770f1babb89492441db0b

@odeke-em
Copy link
Member

odeke-em commented Jul 8, 2017

How's it going @dvyukov? Might you have any bandwidth for this bug? Thanks.

@dvyukov
Copy link
Member Author

dvyukov commented Jul 8, 2017

@odeke-em No, but you don't need to wait for me. We need something simple along the following lines:

--- a/src/runtime/race/testdata/mop_test.go
+++ b/src/runtime/race/testdata/mop_test.go
@@ -9,6 +9,7 @@ import (
        "crypto/sha1"
        "errors"
        "fmt"
+       "internal/race"
        "io"
        "os"
        "runtime"
@@ -62,11 +63,18 @@ func TestRaceIntRWClosures(t *testing.T) {
        var x, y int
        ch := make(chan int, 2)
 
+       nosync := make(chan bool)
        go func() {
                y = x
+               race.Disable()
+               nosync <- true
+               race.Enable()
                ch <- 1
        }()
        go func() {
+               race.Disable()
+               <-nosync
+               race.Enable()
                x = 1
                ch <- 1
        }()

However, I wasn't able to reproduce any flakes on tip. I've tried:

go test -c -race runtime/race/testdata/mop_test.go
stress -failure="PASS" ./race.test -test.run TestRaceIntRWClosures

With different values of -p and also some additional goroutines/computations in the test to defeat 'runnext`.

@dvyukov dvyukov removed their assignment Jul 8, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 14, 2017
@rsc rsc modified the milestones: Go1.10, Unplanned Nov 29, 2017
@gopherbot
Copy link

Change https://golang.org/cl/267998 mentions this issue: runtime/race: remove race from TestNoRaceAfterFunc2

@mpx
Copy link
Contributor

mpx commented Nov 6, 2020

I was just looking into cleaning this up, and AFAICT, TestNoRaceAfterFunc2 is legitimately racey.

The test was added in 908e1b5. I suspect _ = x should be moved before AfterFunc or the test should be removed, but perhaps the original intent was something else?

In practice, the timer function may execute just after the function returns (racing with timer.Stop), or at least, that's when the race is reported:

=== RUN   TestNoRaceAfterFunc2
--- PASS: TestNoRaceAfterFunc2 (0.00s)
=== RUN   TestNoRaceAfterFunc3
==================
WARNING: DATA RACE
Write at 0x00c0000fae60 by goroutine 21:
  command-line-arguments_test.TestNoRaceAfterFunc2.func1()
      .../src/runtime/race/testdata/sync_test.go:130 +0x46
      
Previous read at 0x00c0000fae60 by goroutine 18:
  command-line-arguments_test.TestNoRaceAfterFunc2()
      .../src/runtime/race/testdata/sync_test.go:133 +0x12e
  testing.tRunner()
      .../src/testing/testing.go:1173 +0x202
      
Goroutine 21 (running) created at:
  time.goFunc()
      .../src/time/sleep.go:167 +0x51
      
Goroutine 18 (finished) created at:
  testing.(*T).Run()
      .../src/testing/testing.go:1218 +0x5c7
  testing.runTests.func1()
      .../src/testing/testing.go:1491 +0xa6
  testing.tRunner()
      .../src/testing/testing.go:1173 +0x202
  testing.runTests()
      .../src/testing/testing.go:1489 +0x624
  testing.(*M).Run()
      .../src/testing/testing.go:1397 +0x3b3
  main.main()
      _testmain.go:741 +0x236

gopherbot pushed a commit that referenced this issue Nov 10, 2020
@mpx @ianlancetaylor
runtime/race: remove race from TestNoRaceAfterFunc2
81322b9 
For #14119

Change-Id: I2a9ae43da228cf5c3e38d1f0d1b0768145b6548f
Reviewed-on: https://go-review.googlesource.com/c/go/+/267998
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Run-TryBot: Dmitry Vyukov <dvyukov@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
9 participants
@bradfitz @josharian @rsc @quentinmit @dvyukov @mpx @aclements @odeke-em @gopherbot