Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a race condition when evaluating on the root context #2096

Merged
merged 2 commits into from
Aug 17, 2024

Conversation

wata727
Copy link
Member

@wata727 wata727 commented Aug 11, 2024

Fixes #2094

#1944 parallelized the inspection of child modules, but did not take into account side effects that the child modules could cause on the root module. These are unsafe because the root module is shared by each child module (goroutine).

tflint/cmd/inspect.go

Lines 162 to 174 in 90494f3

// Run checks for module calls are performed in parallel.
// The rootRunner is shared between goroutines but read-only, so this is goroutine-safe.
// Note that checks against the rootRunner are not parallelized, as autofix may cause the module to be rebuilt.
ch := make(chan error, len(moduleRunners))
for _, runner := range moduleRunners {
if opts.NoParallelRunners {
ch <- ruleset.Check(plugin.NewGRPCServer(runner, rootRunner, cli.loader.Files(), sdkVersions[name]))
} else {
go func(runner *tflint.Runner) {
ch <- ruleset.Check(plugin.NewGRPCServer(runner, rootRunner, cli.loader.Files(), sdkVersions[name]))
}(runner)
}
}

The side effect reported in #2094 was the call stack. TFLint detects loops while building the call stack to prevent infinite loops when evaluating local values ​​with circular references. This call stack was shared per Runner (Evaluator).

type Evaluator struct {
Meta *ContextMeta
ModulePath addrs.ModuleInstance
Config *Config
VariableValues map[string]map[string]cty.Value
CallStack *CallStack
}

This means that the call stack with side effects is shared between goroutines.

To prevent this, we moved the call stack into a scope instead of the evaluator. The scope is initialized per evaluation and therefore is not shared between goroutines. As a result, the race condition is fixed.

// EvaluateExpr takes the given HCL expression and evaluates it to produce a value.
func (e *Evaluator) EvaluateExpr(expr hcl.Expression, wantType cty.Type) (cty.Value, hcl.Diagnostics) {
if e == nil {
panic("evaluator must not be nil")
}
return e.scope().EvalExpr(expr, wantType)
}

To prevent such bugs, we added a test case to run with go test --race. In general, this tends to slow down compilation speed, so we only run some tests with --race, and we expect to increase the number of tests if similar bugs are found.

The root runner's Evaluator is shared across goroutines,
so side effects on the CallStack are not goroutine safe.

We solve this problem by moving the CallStack into a Scope
for each evaluation.
@wata727 wata727 merged commit 01b2a57 into master Aug 17, 2024
14 checks passed
@wata727 wata727 deleted the fix_race_condition_on_root_ctx branch August 17, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Race condition on circular dependency check when using parallel workers
1 participant