Skip to content

Concurrent map read/write in common/ast/expr.go #881

Open
#887
Open
Concurrent map read/write in common/ast/expr.go#881
#887
Assignees
TristonianJones
@tonyhb

Description

@tonyhb
tonyhb
opened on Jan 9, 2024

Describe the bug
Evaluating a program concurrently, even in different envs, may fail, when using a cached AST:

==================
WARNING: DATA RACE
Write at 0x00c000d97c58 by goroutine 6297:
  github.com/google/cel-go/common/ast.(*expr).SetKindCase()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/common/ast/expr.go:389 +0x208
  github.com/google/cel-go/checker.(*checker).checkCall()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:233 +0x634
  github.com/google/cel-go/checker.(*checker).check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:90 +0x70
  github.com/google/cel-go/checker.(*checker).checkCall()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:220 +0xf4
  github.com/google/cel-go/checker.(*checker).check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:90 +0x70
  github.com/google/cel-go/checker.(*checker).checkComprehension()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:531 +0xcbc
  github.com/google/cel-go/checker.(*checker).check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:98 +0x84
  github.com/google/cel-go/checker.(*checker).checkCall()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:220 +0xf4
  github.com/google/cel-go/checker.(*checker).check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:90 +0x70
  github.com/google/cel-go/checker.Check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/checker/checker.go:61 +0x3cc
  github.com/google/cel-go/cel.(*Env).Check()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/cel/env.go:224 +0x2d0
  github.com/inngest/expr.(*cachingCompiler).Compile()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/inngest/expr/caching_compiler.go:80 +0x94
  github.com/inngest/inngest/pkg/expressions.NewExpressionEvaluator()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions.go:134 +0x78
  github.com/inngest/inngest/pkg/expressions.Evaluate()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions.go:110 +0x48
  github.com/inngest/inngest/pkg/expressions.TestEvaluateExpression.func1.1()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions_test.go:1006 +0x6c

Previous read at 0x00c000d97c58 by goroutine 6295:
  github.com/google/cel-go/common/ast.(*expr).Kind()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/common/ast/expr.go:316 +0x38
  github.com/google/cel-go/interpreter.(*planner).Plan()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:75 +0x38
  github.com/google/cel-go/interpreter.(*planner).planCall()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:230 +0x1cc
  github.com/google/cel-go/interpreter.(*planner).Plan()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:77 +0x64
  github.com/google/cel-go/interpreter.(*planner).planComprehension()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:593 +0xe4
  github.com/google/cel-go/interpreter.(*planner).Plan()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:91 +0xbc
  github.com/google/cel-go/interpreter.(*planner).planCall()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:230 +0x1cc
  github.com/google/cel-go/interpreter.(*planner).Plan()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/planner.go:77 +0x64
  github.com/google/cel-go/interpreter.(*exprInterpreter).NewInterpretable()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/interpreter/interpreter.go:184 +0x1fc
  github.com/google/cel-go/cel.(*prog).initInterpretable()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/cel/program.go:257 +0x90
  github.com/google/cel-go/cel.newProgram.func1()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/cel/program.go:248 +0xb3c
  github.com/google/cel-go/cel.(*progGen).Eval()
      /home/eng/prj-vm/inngest/inngest/vendor/github.com/google/cel-go/cel/program.go:365 +0x158
  github.com/inngest/inngest/pkg/expressions.eval()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/cel.go:85 +0x84
  github.com/inngest/inngest/pkg/expressions.(*expressionEvaluator).Evaluate()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions.go:248 +0x1d0
  github.com/inngest/inngest/pkg/expressions.Evaluate()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions.go:115 +0xc4
  github.com/inngest/inngest/pkg/expressions.TestEvaluateExpression.func1.1()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions_test.go:1006 +0x6c

Goroutine 6297 (running) created at:
  github.com/inngest/inngest/pkg/expressions.TestEvaluateExpression.func1()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions_test.go:1005 +0x48
  testing.tRunner()
      /home/eng/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /home/eng/go/src/testing/testing.go:1648 +0x40

Goroutine 6295 (finished) created at:
  github.com/inngest/inngest/pkg/expressions.TestEvaluateExpression.func1()
      /home/eng/prj-vm/inngest/inngest/pkg/expressions/expressions_test.go:1005 +0x48
  testing.tRunner()
      /home/eng/go/src/testing/testing.go:1595 +0x194
  testing.(*T).Run.func1()
      /home/eng/go/src/testing/testing.go:1648 +0x40
==================

To Reproduce

  1. Parse an AST
  2. Concurrently evaluate the AST in a new env 100 times in a simple loop:
  3. See the above error.
  • parser
  • checker
  • interpreter

Alternatively, to repro:

git clone [email protected]:inngest/inngest.git
git checkout 1219a37b902649ba87e3d7f1d58aa0f8c224a2c7
go test ./pkg/expressions/ -race

Any expression fails: 5+4 or event.data.name + '-' + event.data.foo.

Expected behavior

Data races don't happen, and we can execute cached ASTs concurrently. Parsing into ASTs via antlr is slow, and we cache strings -> ASTs for speed.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions