concurrency issues in types.Compare() #3793
Comments
|
@markushinz thanks for filing this. Is it possible to share the policy that you used in your test case? I haven't bee able to reproduce the race with large -count values: Test case: package ast
import (
"fmt"
"testing"
)
func getCompiler(modules map[string]*Module) (*Compiler, error) {
compiler := NewCompiler()
compiler.Compile(modules)
if compiler.Failed() {
return nil, compiler.Errors
}
return compiler, nil
}
func getModules(s string) (map[string]*Module, error) {
module, err := ParseModule("test.rego", s)
if err != nil {
return nil, err
}
return map[string]*Module{"test.rego": module}, nil
}
func TestCompileModules(t *testing.T) {
modules, err := getModules(`package test
p { input.bar == input.foo }`)
if err != nil {
t.Fatal(err)
}
for i := 0; i < 10; i++ {
t.Run(fmt.Sprint(i), func(t *testing.T) {
t.Parallel()
_, err := getCompiler(modules)
if err != nil {
t.Fatal(err)
}
})
}
}If you can include the full stack trace, that might also be helpful. |
|
Hi @tsandall, thanks for the quick reply. Here's a concrete example which causes the issue. Just replace func moduleName(m *ast.Module) string {
p := strings.Fields(m.Package.String())
if len(p) != 2 {
return ""
}
return p[len(p)-1]
}
func Test_GetCompiler(t *testing.T) {
rules := map[string]string{
"fileA": "https://github.com/open-policy-agent/library/blob/master/terraform/library.rego", //
"fileB": "https://github.com/open-policy-agent/library/blob/master/terraform/input.rego",
}
modules := make(map[string]*ast.Module)
for _, rule := range rules {
module, err := ast.ParseModule("", rule)
assert.NoError(t, err)
modules[moduleName(module)] = module
}
for i := 0; i < 10; i++ {
t.Run(fmt.Sprintf("compiler_%v", i), func(t *testing.T) {
t.Parallel()
compiler := ast.NewCompiler()
compiler.Compile(modules)
if compiler.Failed() {
assert.NoError(t, compiler.Errors)
}
_, err := getCompiler(modules)
assert.NoError(t, err)
})
}
} |
|
@markushinz thanks. When I run that test case w/ the rego files inlined, I get a data race due to comments on the module not being copied: https://gist.github.com/tsandall/fc863b7a6aca1bfc6940f014a37e1a76. I'll submit a PR with a fix for that. I'm not able to reproduce the original stack trace you shared above. Can you share a stack trace from the full example you provided in your last comment? |
Previously comments were not being deep copied which could lead to data races from the transfomer and other places. Fixes open-policy-agent#3793 Signed-off-by: Torin Sandall <[email protected]>
|
@markushinz if you could try running the test locally in your environment w/ my branch linked in that PR it would be helpful because I'm not able to reproduce the original data race that you showed above. If you post the full stack trace from the race detector that would be super helpful. |
Previously comments were not being deep copied which could lead to data races from the transfomer and other places. Fixes #3793 Signed-off-by: Torin Sandall <[email protected]>
|
Merging the PR closed this. I suppose #3793 (comment) would still be helpful, though @markushinz |
|
@tsandall I did run the tests against your branch and it did not solve the problem. Luckily, I was able to except the rego file which causes the issues. I provided both the full go code (including rego) and the full output of https://gist.github.com/markushinz/a46a48bf74eb8a7176c46502b89bd81c |
This changes updates the implementation of the any type to sort elements during construction. This way the Compare() function does not have to sort elements before recursing (which can result in data races if global type instances from the built-in function declarations or elsewhere are compared.) Fixes #3793 Signed-off-by: Torin Sandall <[email protected]>
|
@markushinz thanks! that was super helpful. I've put up another PR to address the race. 🙏 |
|
@tsandall thanks a lot for addressing this so quickly. Looking forward to testing it once it's merged |
This changes updates the implementation of the any type to sort elements during construction. This way the Compare() function does not have to sort elements before recursing (which can result in data races if global type instances from the built-in function declarations or elsewhere are compared.) Fixes #3793 Signed-off-by: Torin Sandall <[email protected]>
* types: Sort any elements during construction This changes updates the implementation of the any type to sort elements during construction. This way the Compare() function does not have to sort elements before recursing (which can result in data races if global type instances from the built-in function declarations or elsewhere are compared.) Fixes #3793 * capabilities.json: changed ordering * internal/presentation: fix json error output Co-authored-by: Torin Sandall <[email protected]> Co-authored-by: Stephan Renatus <[email protected]> Signed-off-by: Torin Sandall <[email protected]>
|
✔️ @markushinz It's been merged. Please re-open if you find a chance to test this and the issue persists. |
) Previously comments were not being deep copied which could lead to data races from the transfomer and other places. Fixes open-policy-agent#3793 Signed-off-by: Torin Sandall <[email protected]> Signed-off-by: Dolev Farhi <[email protected]>
* types: Sort any elements during construction This changes updates the implementation of the any type to sort elements during construction. This way the Compare() function does not have to sort elements before recursing (which can result in data races if global type instances from the built-in function declarations or elsewhere are compared.) Fixes open-policy-agent#3793 * capabilities.json: changed ordering * internal/presentation: fix json error output Co-authored-by: Torin Sandall <[email protected]> Co-authored-by: Stephan Renatus <[email protected]> Signed-off-by: Torin Sandall <[email protected]> Signed-off-by: Dolev Farhi <[email protected]>
Expected Behavior
I can concurrently create multiple compilers and compile modules. More concrete, I can run a function as the following at the same time in multiple go routines.
Actual Behavior
The above code shows a race condition:
Steps to Reproduce the Problem
To reproduce one can use a test such as the following und run it with
go test -race:Additional Info
The stack trace shows that
types.Compare()causes the issue. It looks like it uses sorting algorithms that directly operate on slices. It usesopa/types/types.go
Line 371 in 4747e22
types.Ais the only issue here, a solution would be to always useopa/types/types.go
Lines 374 to 380 in 4747e22
The text was updated successfully, but these errors were encountered: