Unify locks used on checkers between exclusive pool borrows and EmitResolver scopes #2080
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR unifies the locking mechanism between checker pool borrows and EmitResolver scopes to prevent potential race conditions. The key change is that each Checker now owns a single sync.Mutex that is shared with its EmitResolver, ensuring consistent synchronization across both exclusive pool operations and emit resolver access.
Key Changes:
- Modified
NewCheckerto return both the checker and its lock, making the lock externally accessible - Changed
EmitResolver.checkerMufrom an owned mutex value to a pointer that references the checker's lock - Updated
compiler.checkerPoolto store lock pointers obtained from checkers instead of maintaining separate locks
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/checker.go | Added lock field to Checker struct and modified NewChecker to return the lock alongside the checker instance |
| internal/checker/emitresolver.go | Changed checkerMu from sync.Mutex to *sync.Mutex and initialized it with the checker's lock |
| internal/compiler/checkerpool.go | Changed locks from value slice to pointer slice and populated it with locks returned from NewChecker |
| internal/project/checkerpool.go | Updated NewChecker call to handle the new return signature (lock discarded as this pool doesn't use exclusive locking yet) |
| internal/transformers/tstransforms/importelision_test.go | Updated test code to handle the new NewChecker return signature |
| } | ||
|
|
||
| func NewChecker(program Program) *Checker { | ||
| func NewChecker(program Program) (*Checker, *sync.Mutex) { |
There was a problem hiding this comment.
This feels weird to me; can we just have a method that returns &c.mu? Then callers don't need a separate list of locks either, just the checker they want to lock?
There was a problem hiding this comment.
Or is this intended to force callers to reckon with the locking?
There was a problem hiding this comment.
Precisely that - it forces the callers to reckon with the lock in some way or intentionally discard it. (And basically no caller other than tests should discard it)
2 participants
This is done in the simplest way possible - the checker just makes a lock on construction that is used within the
EmitResolverthe checker creates (instead of the resolver making it) that also gets returned by theNewCheckerconstructor so it can also be used as the canonical lock for that checker by other threadsafe checker API callers.Once again, fixes #2061 maybe - hard to know without a reproduction, but I can definitely see how parallel file emit could invoke both a pooled checker use via incremental/handleNoEmit and an EmitResolver at the same time.