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/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

Comments

@dvyukov
Copy link
Member

@dvyukov 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

@gopherbot gopherbot commented Jan 27, 2016

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
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
@rsc
Copy link
Contributor

@rsc rsc commented Sep 29, 2016

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

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 18, 2017

#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.

@odeke-em
Copy link
Member

@odeke-em 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 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

@gopherbot gopherbot commented Nov 6, 2020

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

@mpx
Copy link
Contributor

@mpx 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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants