Skip to content

Commit d1c8f3b

Browse files
Copilotjakebailey
andcommitted
Fix data race in includeProcessor
Added mutex protection to includeProcessor to prevent concurrent access to processingDiagnostics and fileIncludeReasons. The data race was occurring when multiple goroutines in parseTask.load() called addProcessingDiagnostic simultaneously. Changes: - Added mu sync.Mutex to includeProcessor struct - Protected all reads/writes to processingDiagnostics with mutex - Protected all reads/writes to fileIncludeReasons with mutex - Added addFileIncludeReason() and getFileIncludeReasons() helper methods - Updated all direct field accesses to use protected methods - Used maps.Copy in GetIncludeReasons() as suggested by linter Co-authored-by: jakebailey <[email protected]>
1 parent 8bae50b commit d1c8f3b

File tree

4 files changed

+36
-11
lines changed

4 files changed

+36
-11
lines changed

internal/compiler/filesparser.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -427,10 +427,6 @@ func (w *filesParser) addIncludeReason(loader *fileLoader, task *parseTask, reas
427427
if task.redirectedParseTask != nil {
428428
w.addIncludeReason(loader, task.redirectedParseTask, reason)
429429
} else if task.loaded {
430-
if existing, ok := loader.includeProcessor.fileIncludeReasons[task.path]; ok {
431-
loader.includeProcessor.fileIncludeReasons[task.path] = append(existing, reason)
432-
} else {
433-
loader.includeProcessor.fileIncludeReasons[task.path] = []*FileIncludeReason{reason}
434-
}
430+
loader.includeProcessor.addFileIncludeReason(task.path, reason)
435431
}
436432
}

internal/compiler/includeprocessor.go

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
type includeProcessor struct {
1616
fileIncludeReasons map[tspath.Path][]*FileIncludeReason
1717
processingDiagnostics []*processingDiagnostic
18+
mu sync.Mutex // protects fileIncludeReasons and processingDiagnostics
1819

1920
reasonToReferenceLocation collections.SyncMap[*FileIncludeReason, *referenceFileLocation]
2021
includeReasonToRelatedInfo collections.SyncMap[*FileIncludeReason, *ast.Diagnostic]
@@ -35,7 +36,13 @@ func updateFileIncludeProcessor(p *Program) {
3536
func (i *includeProcessor) getDiagnostics(p *Program) *ast.DiagnosticsCollection {
3637
i.computedDiagnosticsOnce.Do(func() {
3738
i.computedDiagnostics = &ast.DiagnosticsCollection{}
38-
for _, d := range i.processingDiagnostics {
39+
40+
i.mu.Lock()
41+
diagnostics := make([]*processingDiagnostic, len(i.processingDiagnostics))
42+
copy(diagnostics, i.processingDiagnostics)
43+
i.mu.Unlock()
44+
45+
for _, d := range diagnostics {
3946
i.computedDiagnostics.Add(d.toDiagnostic(p))
4047
}
4148
for _, resolutions := range p.resolvedModules {
@@ -57,14 +64,31 @@ func (i *includeProcessor) getDiagnostics(p *Program) *ast.DiagnosticsCollection
5764
}
5865

5966
func (i *includeProcessor) addProcessingDiagnostic(d ...*processingDiagnostic) {
67+
i.mu.Lock()
68+
defer i.mu.Unlock()
6069
i.processingDiagnostics = append(i.processingDiagnostics, d...)
6170
}
6271

72+
func (i *includeProcessor) addFileIncludeReason(path tspath.Path, reason *FileIncludeReason) {
73+
i.mu.Lock()
74+
defer i.mu.Unlock()
75+
i.fileIncludeReasons[path] = append(i.fileIncludeReasons[path], reason)
76+
}
77+
78+
func (i *includeProcessor) getFileIncludeReasons(path tspath.Path) []*FileIncludeReason {
79+
i.mu.Lock()
80+
defer i.mu.Unlock()
81+
return i.fileIncludeReasons[path]
82+
}
83+
6384
func (i *includeProcessor) addProcessingDiagnosticsForFileCasing(file tspath.Path, existingCasing string, currentCasing string, reason *FileIncludeReason) {
85+
i.mu.Lock()
86+
defer i.mu.Unlock()
87+
6488
if !reason.isReferencedFile() && slices.ContainsFunc(i.fileIncludeReasons[file], func(r *FileIncludeReason) bool {
6589
return r.isReferencedFile()
6690
}) {
67-
i.addProcessingDiagnostic(&processingDiagnostic{
91+
i.processingDiagnostics = append(i.processingDiagnostics, &processingDiagnostic{
6892
kind: processingDiagnosticKindExplainingFileInclude,
6993
data: &includeExplainingDiagnostic{
7094
file: file,
@@ -74,7 +98,7 @@ func (i *includeProcessor) addProcessingDiagnosticsForFileCasing(file tspath.Pat
7498
},
7599
})
76100
} else {
77-
i.addProcessingDiagnostic(&processingDiagnostic{
101+
i.processingDiagnostics = append(i.processingDiagnostics, &processingDiagnostic{
78102
kind: processingDiagnosticKindExplainingFileInclude,
79103
data: &includeExplainingDiagnostic{
80104
file: file,

internal/compiler/processingDiagnostic.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func (d *processingDiagnostic) createDiagnosticExplainingFile(program *Program)
9797
// !!! todo sheetal caching
9898

9999
if diag.file != "" {
100-
reasons := program.includeProcessor.fileIncludeReasons[diag.file]
100+
reasons := program.includeProcessor.getFileIncludeReasons(diag.file)
101101
includeDetails = make([]*ast.Diagnostic, 0, len(reasons))
102102
for _, reason := range reasons {
103103
processInclude(reason)

internal/compiler/program.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,7 +1562,12 @@ func (p *Program) GetSourceFiles() []*ast.SourceFile {
15621562

15631563
// Testing only
15641564
func (p *Program) GetIncludeReasons() map[tspath.Path][]*FileIncludeReason {
1565-
return p.includeProcessor.fileIncludeReasons
1565+
p.includeProcessor.mu.Lock()
1566+
defer p.includeProcessor.mu.Unlock()
1567+
// Return a copy to avoid concurrent access issues
1568+
result := make(map[tspath.Path][]*FileIncludeReason, len(p.includeProcessor.fileIncludeReasons))
1569+
maps.Copy(result, p.includeProcessor.fileIncludeReasons)
1570+
return result
15661571
}
15671572

15681573
// Testing only
@@ -1578,7 +1583,7 @@ func (p *Program) ExplainFiles(w io.Writer, locale locale.Locale) {
15781583
}
15791584
for _, file := range p.GetSourceFiles() {
15801585
fmt.Fprintln(w, toRelativeFileName(file.FileName()))
1581-
for _, reason := range p.includeProcessor.fileIncludeReasons[file.Path()] {
1586+
for _, reason := range p.includeProcessor.getFileIncludeReasons(file.Path()) {
15821587
fmt.Fprintln(w, " ", reason.toDiagnostic(p, true).Localize(locale))
15831588
}
15841589
for _, diag := range p.includeProcessor.explainRedirectAndImpliedFormat(p, file, toRelativeFileName) {

0 commit comments

Comments
 (0)