Data race in tests: TestApplicationsUpgradeOverREST race

[algonautshant]

Summary

When multiple threads use the synchTest object, and one fails, all call FailNow.

After the first call, the main test routine reads t.finished to terminate the test.

While the main test routine is reading t.finished, another thread calls FailNow, which in turn writes to t.finished, causing the data race.

While FailNow called from synchTest are guarded by a mutex, the main testing object has the TestingTB object, which does not use the same mutex.

The solution is to call FailNow from synchTest object only the first time.

fixes #2507

More details:

The problem with the string of data race conditions we are observing in different tests when the test fails is the following:

When the test fails, FailNow in go/src/testing/testing.go sets c.finished, which is the "Previous write" part of the data race

func (c *common) FailNow() {
...
	c.finished = true
...

this call comes from different threads, possibly multiple ones, forked from (not exclusively)
go-algorand/nodecontrol/algodControl.go

func (nc *NodeController) StartAlgod(args AlgodStartArgs) (alreadyRunning bool, err error) {
...
	go func() {
...
			if args.ExitErrorCallback != nil {
				args.ExitErrorCallback(nc, err)

where, ExitErrorCallback is:

func (f *LibGoalFixture) nodeExitWithError(nc *nodecontrol.NodeController, err error) {
...
		require.NoError(f.t, err, "Node at %s has terminated with an error", nc.GetDataDir())
...
}

and NoError, in case of an error, calls FailNow.

f.t happens to be synchTest, which has a mutex guarding the FailNow call, and hence, prevents simultaneous access t.finished.

However, there is one more read access to t.finished, which comes from the following location, which does not use synchTest object, hence, does not have the mutex used for the write:

go/src/testing/testing.go

func tRunner(t *T, fn func(t *T)) {
...
	defer func() {
...
		if !t.finished && err == nil {

In other words, when there are multiple threads, and all are fine, no thread will write to t.finished. When something goes wrong and FailNow is initiated, the other threads will be notified of the failure, and, in their turn, also report require.NoError, which calls FailNow, and writes t.finished = true.

Test Plan

[codecov-commenter]

Codecov Report

Merging #2844 (74e55f1) into master (b8b7281) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2844   +/-   ##
=======================================
  Coverage   47.33%   47.33%           
=======================================
  Files         351      351           
  Lines       56523    56523           
=======================================
+ Hits        26754    26756    +2     
+ Misses      26760    26758    -2     
  Partials     3009     3009           

Impacted Files	Coverage Δ
cmd/algoh/blockWatcher.go	77.77% <0.00%> (-3.18%)	⬇️
network/wsPeer.go	74.37% <0.00%> (-0.28%)	⬇️
catchup/service.go	69.35% <0.00%> (+0.77%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8b7281...74e55f1. Read the comment docs.

[tsachiherman]

Based on the details you've provided, I don't believe that the proposed solution would fix the issue:
Consider the case where a new AlgodController is being created, and it's getting returned with an error after the test ends (without reporting any issue ). It would still call the "t.Fail", which would datarace with the tRunner.
I think that we should disable all Fail calls once we've completed the test execution. We can do that using a destructor, in a similar way to SetTestContext()

[algonautshant]

You are right. My changes addressed only failures caused by the test, but not failures coming from the fixture.
This change should address it. I still need to update other tests with this.

[tsachiherman]

looks good. make sure to fix

Go files must be formatted with gofmt. Please run:
  gofmt -w /home/circleci/project/test/e2e-go/upgrades/application_support_test.go

[tsachiherman]

loos good.