Skip to content

Conversation

@weswigham
Copy link
Member

And lock on the cheker in those iterations to make it safe to invoke as many of those (concurrent) iterations concurrently as is required by callers.

Fixes #2061 maybe. I haven't put together a repro for it, but this should fix the race condition in noEmitOnError + incremental the stack trace exposes.

This is basically just the iteration logic from CheckSourceFiles pushed into a helper we use instead of GetAllCheckers with locking added for threadsafety.

Copilot finished reviewing on behalf of weswigham November 12, 2025 21:40
Copy link
Contributor

Copilot AI left a 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 refactors the CheckerPool API to make iteration concurrent by default, replacing GetAllCheckers() with ForEachCheckerParallel(). The change aims to fix a race condition in noEmitOnError + incremental scenarios by adding explicit locking around checker access during parallel iterations.

Key changes:

  • Replaced GetAllCheckers() method with ForEachCheckerParallel() that executes callbacks concurrently with proper locking
  • Added Count() method to CheckerPool interface for pre-allocating result slices
  • Updated all callsites to use the new concurrent iteration pattern with pre-allocated slices and atomic operations for aggregation

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
internal/compiler/checkerpool.go Implements ForEachCheckerParallel with WorkGroup for concurrent execution and per-checker locking; adds Count() method
internal/project/checkerpool.go Implements ForEachCheckerParallel for single-checker scenarios; includes TODO about potential deadlock
internal/compiler/program.go Updates all callsites to use new API; uses atomic operations for counting and pre-allocated slices for diagnostics
internal/testrunner/compiler_runner.go Updates test to use new ForEachCheckerParallel API

@weswigham weswigham enabled auto-merge November 12, 2025 21:52
@weswigham weswigham added this pull request to the merge queue Nov 12, 2025
Merged via the queue into microsoft:main with commit a44d96f Nov 12, 2025
22 checks passed
@weswigham weswigham deleted the safe-implicitly-parallel-checker-work branch November 12, 2025 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HandleNoEmitOnError/GetDiagnosticsOfAnyProgram cause concurrent checker use

3 participants