Skip to content

Commit 04770a4

Browse files
authored
interp: fix data races (#839)
This change fixes two distinct data races: 1) some global vars of type *itype of the interp package are actually mutated during the lifecycle of an Interpreter. Even worse: if more than one Interpreter instance are created and used at a given time, they are actually racing each other for these global vars. Therefore, this change replaces these global vars with generator functions that create the needed type on the fly. 2) the symbols given as argument of Interpreter.Use were directly copied as reference (since they're maps) when mapped inside an Interpreter instance. Since the usual case is to give the symbols from the stdlib package, it means when the interpreter mutates its own symbols in fixStdio, it would actually mutate the corresponding global vars of the stdlib package. Again, this is at least racy as soon as several instances of an Intepreter are concurrently running. This change fixes the race by making sure Interpreter.Use actually copies the symbol values instead of copying the references.
1 parent 341c69d commit 04770a4

File tree

5 files changed

+235
-22
lines changed

5 files changed

+235
-22
lines changed

interp/interp.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -319,9 +319,9 @@ func initUniverse() *scope {
319319
"uintptr": {kind: typeSym, typ: &itype{cat: uintptrT, name: "uintptr"}},
320320

321321
// predefined Go constants
322-
"false": {kind: constSym, typ: untypedBool, rval: reflect.ValueOf(false)},
323-
"true": {kind: constSym, typ: untypedBool, rval: reflect.ValueOf(true)},
324-
"iota": {kind: constSym, typ: untypedInt},
322+
"false": {kind: constSym, typ: untypedBool(), rval: reflect.ValueOf(false)},
323+
"true": {kind: constSym, typ: untypedBool(), rval: reflect.ValueOf(true)},
324+
"iota": {kind: constSym, typ: untypedInt()},
325325

326326
// predefined Go zero value
327327
"nil": {typ: &itype{cat: nilT, untyped: true}},
@@ -561,8 +561,7 @@ func (interp *Interpreter) Use(values Exports) {
561561
}
562562

563563
if interp.binPkg[k] == nil {
564-
interp.binPkg[k] = v
565-
continue
564+
interp.binPkg[k] = make(map[string]reflect.Value)
566565
}
567566

568567
for s, sym := range v {
@@ -571,7 +570,7 @@ func (interp *Interpreter) Use(values Exports) {
571570
}
572571

573572
// Checks if input values correspond to stdlib packages by looking for one
574-
// well knwonw stdlib package path.
573+
// well known stdlib package path.
575574
if _, ok := values["fmt"]; ok {
576575
fixStdio(interp)
577576
}

interp/interp_eval_test.go

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package interp_test
22

33
import (
4+
"bufio"
45
"bytes"
56
"context"
67
"fmt"
@@ -917,6 +918,199 @@ func TestImportPathIsKey(t *testing.T) {
917918
}
918919
}
919920

921+
// The code in hello1.go and hello2.go spawns a "long-running" goroutine, which
922+
// means each call to EvalPath actually terminates before the evaled code is done
923+
// running. So this test demonstrates:
924+
// 1) That two sequential calls to EvalPath don't see their "compilation phases"
925+
// collide (no data race on the fields of the interpreter), which is somewhat
926+
// obvious since the calls (and hence the "compilation phases") are sequential too.
927+
// 2) That two concurrent goroutine runs spawned by the same interpreter do not
928+
// collide either.
929+
func TestConcurrentEvals(t *testing.T) {
930+
if testing.Short() {
931+
return
932+
}
933+
pin, pout := io.Pipe()
934+
defer func() {
935+
_ = pin.Close()
936+
_ = pout.Close()
937+
}()
938+
interpr := interp.New(interp.Options{Stdout: pout})
939+
interpr.Use(stdlib.Symbols)
940+
941+
if _, err := interpr.EvalPath("testdata/concurrent/hello1.go"); err != nil {
942+
t.Fatal(err)
943+
}
944+
if _, err := interpr.EvalPath("testdata/concurrent/hello2.go"); err != nil {
945+
t.Fatal(err)
946+
}
947+
948+
c := make(chan error)
949+
go func() {
950+
hello1, hello2 := false, false
951+
sc := bufio.NewScanner(pin)
952+
for sc.Scan() {
953+
l := sc.Text()
954+
switch l {
955+
case "hello world1":
956+
hello1 = true
957+
case "hello world2":
958+
hello2 = true
959+
default:
960+
c <- fmt.Errorf("unexpected output: %v", l)
961+
return
962+
}
963+
if hello1 && hello2 {
964+
break
965+
}
966+
}
967+
c <- nil
968+
}()
969+
970+
timeout := time.NewTimer(5 * time.Second)
971+
select {
972+
case <-timeout.C:
973+
t.Fatal("timeout")
974+
case err := <-c:
975+
if err != nil {
976+
t.Fatal(err)
977+
}
978+
}
979+
}
980+
981+
// TestConcurrentEvals2 shows that even though EvalWithContext calls Eval in a
982+
// goroutine, it indeed waits for Eval to terminate, and that therefore the code
983+
// called by EvalWithContext is sequential. And that there is no data race for the
984+
// interp package global vars or the interpreter fields in this case.
985+
func TestConcurrentEvals2(t *testing.T) {
986+
pin, pout := io.Pipe()
987+
defer func() {
988+
_ = pin.Close()
989+
_ = pout.Close()
990+
}()
991+
interpr := interp.New(interp.Options{Stdout: pout})
992+
interpr.Use(stdlib.Symbols)
993+
994+
done := make(chan error)
995+
go func() {
996+
hello1 := false
997+
sc := bufio.NewScanner(pin)
998+
for sc.Scan() {
999+
l := sc.Text()
1000+
if hello1 {
1001+
if l == "hello world2" {
1002+
break
1003+
} else {
1004+
done <- fmt.Errorf("unexpected output: %v", l)
1005+
return
1006+
}
1007+
}
1008+
if l == "hello world1" {
1009+
hello1 = true
1010+
} else {
1011+
done <- fmt.Errorf("unexpected output: %v", l)
1012+
return
1013+
}
1014+
}
1015+
done <- nil
1016+
}()
1017+
1018+
ctx := context.Background()
1019+
if _, err := interpr.EvalWithContext(ctx, `import "time"`); err != nil {
1020+
t.Fatal(err)
1021+
}
1022+
if _, err := interpr.EvalWithContext(ctx, `time.Sleep(time.Second); println("hello world1")`); err != nil {
1023+
t.Fatal(err)
1024+
}
1025+
if _, err := interpr.EvalWithContext(ctx, `time.Sleep(time.Second); println("hello world2")`); err != nil {
1026+
t.Fatal(err)
1027+
}
1028+
1029+
timeout := time.NewTimer(5 * time.Second)
1030+
select {
1031+
case <-timeout.C:
1032+
t.Fatal("timeout")
1033+
case err := <-done:
1034+
if err != nil {
1035+
t.Fatal(err)
1036+
}
1037+
}
1038+
}
1039+
1040+
// TestConcurrentEvals3 makes sure that we don't regress into data races at the package level, i.e from:
1041+
// - global vars, which should obviously not be mutated.
1042+
// - when calling Interpreter.Use, the symbols given as argument should be
1043+
// copied when being inserted into interp.binPkg, and not directly used as-is.
1044+
func TestConcurrentEvals3(t *testing.T) {
1045+
allDone := make(chan bool)
1046+
runREPL := func() {
1047+
done := make(chan error)
1048+
pinin, poutin := io.Pipe()
1049+
pinout, poutout := io.Pipe()
1050+
i := interp.New(interp.Options{Stdin: pinin, Stdout: poutout})
1051+
i.Use(stdlib.Symbols)
1052+
1053+
go func() {
1054+
_, _ = i.REPL()
1055+
}()
1056+
1057+
input := []string{
1058+
`hello one`,
1059+
`hello two`,
1060+
`hello three`,
1061+
}
1062+
1063+
go func() {
1064+
sc := bufio.NewScanner(pinout)
1065+
k := 0
1066+
for sc.Scan() {
1067+
l := sc.Text()
1068+
if l != input[k] {
1069+
done <- fmt.Errorf("unexpected output, want %q, got %q", input[k], l)
1070+
return
1071+
}
1072+
k++
1073+
if k > 2 {
1074+
break
1075+
}
1076+
}
1077+
done <- nil
1078+
}()
1079+
1080+
for _, v := range input {
1081+
in := strings.NewReader(fmt.Sprintf("println(\"%s\")\n", v))
1082+
if _, err := io.Copy(poutin, in); err != nil {
1083+
t.Fatal(err)
1084+
}
1085+
time.Sleep(time.Second)
1086+
}
1087+
1088+
if err := <-done; err != nil {
1089+
t.Fatal(err)
1090+
}
1091+
_ = pinin.Close()
1092+
_ = poutin.Close()
1093+
_ = pinout.Close()
1094+
_ = poutout.Close()
1095+
allDone <- true
1096+
}
1097+
1098+
for i := 0; i < 2; i++ {
1099+
go func() {
1100+
runREPL()
1101+
}()
1102+
}
1103+
1104+
timeout := time.NewTimer(10 * time.Second)
1105+
for i := 0; i < 2; i++ {
1106+
select {
1107+
case <-allDone:
1108+
case <-timeout.C:
1109+
t.Fatal("timeout")
1110+
}
1111+
}
1112+
}
1113+
9201114
func TestEvalScanner(t *testing.T) {
9211115
type testCase struct {
9221116
desc string
@@ -1005,6 +1199,7 @@ func TestEvalScanner(t *testing.T) {
10051199
}
10061200

10071201
runREPL := func(t *testing.T, test testCase) {
1202+
// TODO(mpl): use a pipe for the output as well, just as in TestConcurrentEvals5
10081203
var stdout bytes.Buffer
10091204
safeStdout := &safeBuffer{buf: &stdout}
10101205
var stderr bytes.Buffer
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
package main
2+
3+
import "time"
4+
5+
func main() {
6+
go func() {
7+
time.Sleep(3 * time.Second)
8+
println("hello world1")
9+
}()
10+
}
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
package main
2+
3+
import "time"
4+
5+
func main() {
6+
go func() {
7+
time.Sleep(3 * time.Second)
8+
println("hello world2")
9+
}()
10+
}

interp/type.go

Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -125,14 +125,12 @@ type itype struct {
125125
scope *scope // type declaration scope (in case of re-parse incomplete type)
126126
}
127127

128-
var (
129-
untypedBool = &itype{cat: boolT, name: "bool", untyped: true}
130-
untypedString = &itype{cat: stringT, name: "string", untyped: true}
131-
untypedRune = &itype{cat: int32T, name: "int32", untyped: true}
132-
untypedInt = &itype{cat: intT, name: "int", untyped: true}
133-
untypedFloat = &itype{cat: float64T, name: "float64", untyped: true}
134-
untypedComplex = &itype{cat: complex128T, name: "complex128", untyped: true}
135-
)
128+
func untypedBool() *itype { return &itype{cat: boolT, name: "bool", untyped: true} }
129+
func untypedString() *itype { return &itype{cat: stringT, name: "string", untyped: true} }
130+
func untypedRune() *itype { return &itype{cat: int32T, name: "int32", untyped: true} }
131+
func untypedInt() *itype { return &itype{cat: intT, name: "int", untyped: true} }
132+
func untypedFloat() *itype { return &itype{cat: float64T, name: "float64", untyped: true} }
133+
func untypedComplex() *itype { return &itype{cat: complex128T, name: "complex128", untyped: true} }
136134

137135
// nodeType returns a type definition for the corresponding AST subtree.
138136
func nodeType(interp *Interpreter, sc *scope, n *node) (*itype, error) {
@@ -221,24 +219,24 @@ func nodeType(interp *Interpreter, sc *scope, n *node) (*itype, error) {
221219
switch v := n.rval.Interface().(type) {
222220
case bool:
223221
n.rval = reflect.ValueOf(constant.MakeBool(v))
224-
t = untypedBool
222+
t = untypedBool()
225223
case rune:
226224
// It is impossible to work out rune const literals in AST
227225
// with the correct type so we must make the const type here.
228226
n.rval = reflect.ValueOf(constant.MakeInt64(int64(v)))
229-
t = untypedRune
227+
t = untypedRune()
230228
case constant.Value:
231229
switch v.Kind() {
232230
case constant.Bool:
233-
t = untypedBool
231+
t = untypedBool()
234232
case constant.String:
235-
t = untypedString
233+
t = untypedString()
236234
case constant.Int:
237-
t = untypedInt
235+
t = untypedInt()
238236
case constant.Float:
239-
t = untypedFloat
237+
t = untypedFloat()
240238
case constant.Complex:
241-
t = untypedComplex
239+
t = untypedComplex()
242240
default:
243241
err = n.cfgErrorf("missing support for type %v", n.rval)
244242
}
@@ -299,7 +297,7 @@ func nodeType(interp *Interpreter, sc *scope, n *node) (*itype, error) {
299297
case isFloat64(t0) && isFloat64(t1):
300298
t = sc.getType("complex128")
301299
case nt0.untyped && isNumber(t0) && nt1.untyped && isNumber(t1):
302-
t = untypedComplex
300+
t = untypedComplex()
303301
case nt0.untyped && isFloat32(t1) || nt1.untyped && isFloat32(t0):
304302
t = sc.getType("complex64")
305303
case nt0.untyped && isFloat64(t1) || nt1.untyped && isFloat64(t0):
@@ -1302,6 +1300,7 @@ func exportName(s string) string {
13021300
}
13031301

13041302
var (
1303+
// TODO(mpl): generators.
13051304
interf = reflect.TypeOf((*interface{})(nil)).Elem()
13061305
constVal = reflect.TypeOf((*constant.Value)(nil)).Elem()
13071306
)

0 commit comments

Comments
 (0)