Skip to content

Commit

Permalink
TestEvalScanner: "fix" data race
Browse files Browse the repository at this point in the history
When running TestEvalScanner with -race=true, one can observe a data race such as:

```
WARNING: DATA RACE
Read at 0x0000029f3d68 by goroutine 52:
  github.com/containous/yaegi/interp.(*itype).defaultType()
      /Users/mpl/src/github.com/containous/yaegi/interp/type.go:1466 +0x572
...
  github.com/containous/yaegi/interp.(*Interpreter).EvalWithContext.func1()
      /Users/mpl/src/github.com/containous/yaegi/interp/interp.go:501 +0xf0

Previous write at 0x0000029f3d68 by goroutine 43:
  github.com/containous/yaegi/interp.(*itype).refType()
      /Users/mpl/src/github.com/containous/yaegi/interp/type.go:1419 +0x854
  github.com/containous/yaegi/interp.(*itype).TypeOf()
      /Users/mpl/src/github.com/containous/yaegi/interp/type.go:1427 +0xa6
...
  github.com/containous/yaegi/interp.(*Interpreter).EvalWithContext.func1()
      /Users/mpl/src/github.com/containous/yaegi/interp/interp.go:501 +0xf0
```

Before this change, since closing the pipe to the REPL is done in a defer, it
means that all the i.REPL calls (and hence each goroutine for each of these
calls) are kept running and alive until the very end of the test. It should not
matter, since a new interpreter is created for each test case, and thus all the
i.REPL calls should be completely independent from each other.

And yet, by wrapping each test case in a function call, and thus making each
i.REPL call terminate as soon as the test case is over, the data race seems to
be fixed. This could suggest that the separate i.REPL calls from separate
interpreter instances are somehow sharing some memory, but I do not know how to
explain that.

The problem has yet to be fully understood, but at least this change restores
the test, without making the CI fail again.
  • Loading branch information
mpl committed Aug 31, 2020
1 parent 535e7e1 commit f4cc059
Showing 1 changed file with 33 additions and 31 deletions.
64 changes: 33 additions & 31 deletions interp/interp_eval_test.go
@@ -1,15 +1,19 @@
package interp_test

import (
"bytes"
"context"
"fmt"
"io"
"io/ioutil"
"log"
"net/http"
"os"
"path/filepath"
"reflect"
"strconv"
"strings"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -899,15 +903,13 @@ func TestImportPathIsKey(t *testing.T) {
}
}

// Disabled for now, because it reveals a data race, and we want our current PRs
// to pass the CI.
/*
func TestEvalScanner(t *testing.T) {
tests := []struct {
type testCase struct {
desc string
src []string
errorLine int
}{
}
tests := []testCase{
{
desc: "no error",
src: []string{
Expand Down Expand Up @@ -962,36 +964,33 @@ func TestEvalScanner(t *testing.T) {
},
errorLine: -1,
},
// TODO: these tests are showing that we handle retries properly for func
// closure calls, but they reveal a data race we seem to have in EvalWithContext,
// so they're disabled for now to avoid the CI nagging us about it.
{
desc: "anonymous func call with no assignment",
src: []string{
`func() { println(3) }()`,
},
errorLine: -1,
{
desc: "anonymous func call with no assignment",
src: []string{
`func() { println(3) }()`,
},
{
// to make sure that special handling of the above anonymous, does not break this general case.
desc: "just func",
src: []string{
`func foo() { println(3) }`,
},
errorLine: -1,
errorLine: -1,
},
{
// to make sure that special handling of the above anonymous, does not break this general case.
desc: "just func",
src: []string{
`func foo() { println(3) }`,
},
{
// to make sure that special handling of the above anonymous, does not break this general case.
desc: "just method",
src: []string{
`type bar string`,
`func (b bar) foo() { println(3) }`,
},
errorLine: -1,
errorLine: -1,
},
{
// to make sure that special handling of the above anonymous, does not break this general case.
desc: "just method",
src: []string{
`type bar string`,
`func (b bar) foo() { println(3) }`,
},
errorLine: -1,
},
}

for _, test := range tests {
runREPL := func(t *testing.T, test testCase) {
i := interp.New(interp.Options{})
var stderr bytes.Buffer
safeStderr := &safeBuffer{buf: &stderr}
Expand Down Expand Up @@ -1024,6 +1023,10 @@ func TestEvalScanner(t *testing.T) {
}
}
}

for _, test := range tests {
runREPL(t, test)
}
}

type safeBuffer struct {
Expand Down Expand Up @@ -1069,4 +1072,3 @@ func applyCIMultiplier(timeout time.Duration) time.Duration {
}
return time.Duration(float64(timeout) * CITimeoutMultiplier)
}
*/

0 comments on commit f4cc059

Please sign in to comment.