controllogic: fix data race in setPath nested-array assignment
setPath mutated nested sub-arrays in place; since graph-local slices (and their nested sub-slices) may be read concurrently by other flow goroutines, an action.array.set with a multi-level path raced readers of the same local. The single-threaded TS source mutates in place safely, but the Go port runs flows on concurrent goroutines, so setPath now copies on descent (every level returns a freshly allocated slice). Adds a -race regression test that concurrently drives nested set + inner-element reads, plus a nested-set correctness test asserting the prior stored value is not mutated. Found by whole-branch review (Opus); missed by the per-task reviews. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -277,7 +277,11 @@ func (e *Engine) liveGet(ds, name string) float64 {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// setPath assigns v at the nested index path within arr, growing (zero-filling)
|
// setPath assigns v at the nested index path within arr, growing (zero-filling)
|
||||||
// as needed and resolving negative indices against length. Port of logic.ts.
|
// as needed and resolving negative indices against length. Adapted from logic.ts;
|
||||||
|
// unlike the single-threaded TS source it copies on descent rather than mutating in
|
||||||
|
// place, because a graph-local slice (and its nested sub-slices) may be read
|
||||||
|
// concurrently by other flow goroutines — in-place mutation of shared backing would
|
||||||
|
// race. Every level returns a freshly allocated slice.
|
||||||
func setPath(arr []Value, path []int, v Value) []Value {
|
func setPath(arr []Value, path []int, v Value) []Value {
|
||||||
if len(path) == 0 {
|
if len(path) == 0 {
|
||||||
return arr
|
return arr
|
||||||
@@ -289,16 +293,17 @@ func setPath(arr []Value, path []int, v Value) []Value {
|
|||||||
if k < 0 {
|
if k < 0 {
|
||||||
return arr
|
return arr
|
||||||
}
|
}
|
||||||
for len(arr) <= k {
|
out := append([]Value{}, arr...) // copy: backing may be shared with the live local
|
||||||
arr = append(arr, 0.0)
|
for len(out) <= k {
|
||||||
|
out = append(out, 0.0)
|
||||||
}
|
}
|
||||||
if len(path) == 1 {
|
if len(path) == 1 {
|
||||||
arr[k] = v
|
out[k] = v
|
||||||
return arr
|
return out
|
||||||
}
|
}
|
||||||
sub, _ := arr[k].([]Value)
|
sub, _ := out[k].([]Value)
|
||||||
arr[k] = setPath(sub, path[1:], v)
|
out[k] = setPath(sub, path[1:], v)
|
||||||
return arr
|
return out
|
||||||
}
|
}
|
||||||
|
|
||||||
// write applies an action.write/lua-set to a target: a bare name updates a
|
// write applies an action.write/lua-set to a target: a bare name updates a
|
||||||
|
|||||||
@@ -3,6 +3,7 @@ package controllogic
|
|||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
"reflect"
|
"reflect"
|
||||||
|
"sync"
|
||||||
"testing"
|
"testing"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -108,6 +109,79 @@ func TestArrayPopNode(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestArraySetNodeNested verifies set with a comma-separated path mutates a nested
|
||||||
|
// sub-array, and (crucially) that setPath does not mutate the previously stored slice
|
||||||
|
// in place — the stored value must be replaced by a fresh tree.
|
||||||
|
func TestArraySetNodeNested(t *testing.T) {
|
||||||
|
g := Graph{
|
||||||
|
ID: "g", Name: "n",
|
||||||
|
StateVars: []StateVar{{Name: "grid", Type: "array", Initial: "[[1,2],[3,4]]", Sizing: "dynamic"}},
|
||||||
|
Nodes: []Node{
|
||||||
|
{ID: "t", Kind: "trigger.timer", Params: map[string]string{"interval": "1000"}},
|
||||||
|
{ID: "s", Kind: "action.array.set", Params: map[string]string{"array": "grid", "index": "0,1", "expr": "9"}},
|
||||||
|
},
|
||||||
|
Wires: []Wire{{From: "t", To: "s"}},
|
||||||
|
}
|
||||||
|
cg := compile(g)
|
||||||
|
before := cg.getLocal("grid")
|
||||||
|
cg.engine = &Engine{}
|
||||||
|
cg.genCtx = context.Background()
|
||||||
|
R := func(ds, name string) Value {
|
||||||
|
if ds == "local" {
|
||||||
|
return cg.getLocal(name)
|
||||||
|
}
|
||||||
|
return 0.0
|
||||||
|
}
|
||||||
|
cg.follow("t", "out", &runCtx{fired: "t", resolve: R})
|
||||||
|
want := []Value{[]Value{1.0, 9.0}, []Value{3.0, 4.0}}
|
||||||
|
if got := cg.getLocal("grid"); !reflect.DeepEqual(got, want) {
|
||||||
|
t.Fatalf("after nested set grid = %#v", got)
|
||||||
|
}
|
||||||
|
// The pre-mutation snapshot must be untouched (no shared-backing in-place write).
|
||||||
|
if !reflect.DeepEqual(before, []Value{[]Value{1.0, 2.0}, []Value{3.0, 4.0}}) {
|
||||||
|
t.Fatalf("setPath mutated the prior stored value in place: %#v", before)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestArraySetNodeConcurrentNoRace drives many concurrent set+read flows against the
|
||||||
|
// same nested-array local; with -race it guards the setPath copy-on-descend fix.
|
||||||
|
func TestArraySetNodeConcurrentNoRace(t *testing.T) {
|
||||||
|
g := Graph{
|
||||||
|
ID: "g", Name: "n",
|
||||||
|
StateVars: []StateVar{{Name: "grid", Type: "array", Initial: "[[0,0],[0,0]]", Sizing: "dynamic"}},
|
||||||
|
Nodes: []Node{
|
||||||
|
{ID: "t", Kind: "trigger.timer", Params: map[string]string{"interval": "1000"}},
|
||||||
|
{ID: "s", Kind: "action.array.set", Params: map[string]string{"array": "grid", "index": "0,0", "expr": "1"}},
|
||||||
|
},
|
||||||
|
Wires: []Wire{{From: "t", To: "s"}},
|
||||||
|
}
|
||||||
|
cg := compile(g)
|
||||||
|
cg.engine = &Engine{}
|
||||||
|
cg.genCtx = context.Background()
|
||||||
|
R := func(ds, name string) Value {
|
||||||
|
if ds == "local" {
|
||||||
|
return cg.getLocal(name)
|
||||||
|
}
|
||||||
|
return 0.0
|
||||||
|
}
|
||||||
|
var wg sync.WaitGroup
|
||||||
|
for i := 0; i < 32; i++ {
|
||||||
|
wg.Add(2)
|
||||||
|
go func() { defer wg.Done(); cg.follow("t", "out", &runCtx{fired: "t", resolve: R}) }()
|
||||||
|
go func() {
|
||||||
|
defer wg.Done()
|
||||||
|
// Read the inner element concurrently; with the in-place setPath bug the
|
||||||
|
// writer mutates this same sub-array backing, producing a data race.
|
||||||
|
if arr, ok := cg.getLocal("grid").([]Value); ok && len(arr) > 0 {
|
||||||
|
if sub, ok := arr[0].([]Value); ok && len(sub) > 0 {
|
||||||
|
_ = sub[0]
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}()
|
||||||
|
}
|
||||||
|
wg.Wait()
|
||||||
|
}
|
||||||
|
|
||||||
func TestCompileInitsLocalsFromDecls(t *testing.T) {
|
func TestCompileInitsLocalsFromDecls(t *testing.T) {
|
||||||
g := Graph{
|
g := Graph{
|
||||||
ID: "g", Name: "n",
|
ID: "g", Name: "n",
|
||||||
|
|||||||
Reference in New Issue
Block a user