-
Notifications
You must be signed in to change notification settings - Fork 734
Add locks on concurrent alias following checker accesses under incremental mode #2051
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
Add locks on concurrent alias following checker accesses under incremental mode #2051
Conversation
…e checker access tracked by checker pools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a race condition in incremental compilation mode where concurrent access to the same checker for alias following operations could cause data races. The fix introduces thread-safe exclusive checker access via locks and enables incremental mode testing through compiler tests with @incremental: true.
- Adds
GetCheckerForFileExclusivemethods to checker pool interfaces, which lock a mutex per checker to prevent concurrent access - Updates
affectedfileshandler.goto use exclusive checker access for alias following operations - Extends the
ProgramLikeinterface with additional methods needed for incremental program compatibility - Enables incremental mode in test infrastructure when
@incremental: trueis specified
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts | New regression test with multiple files and export aliases to reproduce the race condition |
| testdata/baselines/reference/compiler/incrementalConcurrentSafeAliasFollowing.* | Expected baseline outputs for the new test |
| internal/project/checkerpool.go | Adds per-checker mutex locks and GetCheckerForFileExclusive implementation for the project-level checker pool |
| internal/compiler/checkerpool.go | Adds per-checker mutex locks and GetCheckerForFileExclusive implementation for the compiler-level checker pool |
| internal/compiler/program.go | Exposes GetCheckerForFileExclusive and extends ProgramLike interface |
| internal/execute/incremental/affectedfileshandler.go | Uses exclusive checker access to prevent race condition during alias following |
| internal/execute/incremental/program.go | Implements additional ProgramLike interface methods for compatibility |
| internal/testutil/harnessutil/harnessutil.go | Enables incremental program construction in tests, changes Program field to ProgramLike |
| internal/testutil/tsbaseline/*.go | Updates to work with ProgramLike interface instead of concrete Program type |
| internal/testrunner/compiler_runner.go | Adds type switching logic to extract underlying Program from ProgramLike |
|
|
||
| func (p *checkerPool) GetCheckerForFileExclusive(ctx context.Context, file *ast.SourceFile) (*checker.Checker, func()) { | ||
| c, done := p.GetCheckerForFile(ctx, file) | ||
| idx := slices.Index(p.checkers, c) |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for when slices.Index returns -1 (checker not found). If the checker is not in the slice, this will cause a panic when accessing p.locks[idx]. Although this should never happen in normal operation, defensive programming suggests adding a check or assertion.
| idx := slices.Index(p.checkers, c) | |
| idx := slices.Index(p.checkers, c) | |
| if idx == -1 { | |
| panic("checker not found in checker pool") | |
| } |
|
|
||
| //// [a.tsbuildinfo] | ||
| { | ||
| "version": "TEST", |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: Unexpected token
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
testdata/tests/cases/compiler/incrementalConcurrentSafeAliasFollowing.ts
Show resolved
Hide resolved
This reverts commit b42a8b5.
And, so we can test for these races more simply, enable the incremental mode program construction+emit codepath for compiler tests with
@incremental: truespecified.There are other
GetCheckerForFilecalls ininternal/execute/incrementalthat probably need to be swapped toGetCheckerForFileExclusive, as using basically any checker functions in a concurrent context without a lock on it could lead to a data race (thanks to lazy caching everywhere inside the checker), but the single call I changed here is definitely the root cause of the crash in the issue. I could just go swap them all, but we don't have regression tests for those sites to prove that's actually needed. The LS has request-scoped checker associations and assumes one thread per request, so is unlikely to need the per-checker lock functionality (though it is here, since I made it part of the pool interface), unless the number of in-flight requests exceeds the maximum number of available checkers (which can only happen if there's a disconnect between request workgroup max paralleism and max checkers afaik).Fixes #1470