[LSP] Fix data races on dirty state and script info version #897
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses data race issues on dirty state and improvements to script info version usage by consolidating mutexes and reducing redundant method calls during file parsing.
- Replace multiple mutexes with a single mutex for project state.
- Remove redundant symlink-related method calls in script info.
- Cache script info version and text to avoid multiple evaluations.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/project/scriptinfo.go | Removed redundant calls and eliminated an unused isSymlink function. |
| internal/project/project.go | Consolidated mutexes and refactored logging based on updated file changes. |
| internal/project/documentregistry.go | Cached script info version and text for consistent file parsing. |
Comments suppressed due to low confidence (3)
internal/project/scriptinfo.go:125
- Ensure that removing the symlink notification call aligns with the overall design and that any side effects previously handled by this call are now covered elsewhere.
project.onFileAddedOrRemoved(s.isSymlink())
internal/project/project.go:95
- [nitpick] Since the mutex now governs multiple aspects of project state, consider renaming 'mu' to something more descriptive (e.g., 'stateMu') to clarify its purpose.
mu sync.Mutex
internal/project/project.go:114
- While consolidating mutexes simplifies synchronization, verify that p.mu adequately protects the program field to avoid contention or race conditions previously managed by programMu.
programMu sync.Mutex
| func (p *Project) onFileAddedOrRemoved(isSymlink bool) { | ||
| p.dirtyStateMu.Lock() | ||
| defer p.dirtyStateMu.Unlock() | ||
| p.hasAddedOrRemovedFiles = true |
There was a problem hiding this comment.
These weren’t yet used for anything but logging, and they complicate the locking because they are sometimes, but not always, called during program construction when the mutex is already locked.
| scriptInfoVersion := scriptInfo.Version() | ||
| scriptInfoText := scriptInfo.Text() |
There was a problem hiding this comment.
Technically this should be one atomic access
There was a problem hiding this comment.
Do you want to address this now or later?
There was a problem hiding this comment.
I just tried protecting all the ScriptInfo fields with a mutex and accessing correlated fields within the same lock, but that had a ~20% penalty on project initialization
| scriptInfoVersion := scriptInfo.Version() | ||
| scriptInfoText := scriptInfo.Text() |
There was a problem hiding this comment.
Do you want to address this now or later?
Changes
projectto operate with a single mutex, and avoids readingscriptInfo.Version()multiple times during parsing.