Skip to content

Commit 52190eb

Browse files
andrewbranchandrewbranch
authored
[LSP] Fix data races on dirty state and script info version (microsoft#897)
1 parent 34dbdc4 commit 52190eb

File tree

4 files changed

+72
-68
lines changed

4 files changed

+72
-68
lines changed

‎internal/project/documentregistry.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,14 @@ func (r *DocumentRegistry) getDocumentWorker(
8989
key registryKey,
9090
) *ast.SourceFile {
9191
scriptTarget := core.IfElse(scriptInfo.scriptKind == core.ScriptKindJSON, core.ScriptTargetJSON, compilerOptions.GetEmitScriptTarget())
92+
scriptInfoVersion := scriptInfo.Version()
93+
scriptInfoText := scriptInfo.Text()
9294
if entry, ok := r.documents.Load(key); ok {
9395
// We have an entry for this file. However, it may be for a different version of
9496
// the script snapshot. If so, update it appropriately.
95-
if entry.sourceFile.Version != scriptInfo.Version() {
96-
sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfo.text, scriptTarget, scanner.JSDocParsingModeParseAll)
97-
sourceFile.Version = scriptInfo.Version()
97+
if entry.sourceFile.Version != scriptInfoVersion {
98+
sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfoText, scriptTarget, scanner.JSDocParsingModeParseAll)
99+
sourceFile.Version = scriptInfoVersion
98100
entry.mu.Lock()
99101
defer entry.mu.Unlock()
100102
entry.sourceFile = sourceFile
@@ -103,8 +105,8 @@ func (r *DocumentRegistry) getDocumentWorker(
103105
return entry.sourceFile
104106
} else {
105107
// Have never seen this file with these settings. Create a new source file for it.
106-
sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfo.text, scriptTarget, scanner.JSDocParsingModeParseAll)
107-
sourceFile.Version = scriptInfo.Version()
108+
sourceFile := parser.ParseSourceFile(scriptInfo.fileName, scriptInfo.path, scriptInfoText, scriptTarget, scanner.JSDocParsingModeParseAll)
109+
sourceFile.Version = scriptInfoVersion
108110
entry, _ := r.documents.LoadOrStore(key, &registryEntry{
109111
sourceFile: sourceFile,
110112
refCount: 0,

‎internal/project/project.go

Lines changed: 56 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"slices"
88
"strings"
99
"sync"
10+
"time"
1011

1112
"github.com/microsoft/typescript-go/internal/ast"
1213
"github.com/microsoft/typescript-go/internal/collections"
@@ -92,15 +93,13 @@ type Project struct {
9293
name string
9394
kind Kind
9495

95-
dirtyStateMu sync.Mutex
96-
initialLoadPending bool
97-
dirty bool
98-
version int
99-
hasAddedOrRemovedFiles bool
100-
hasAddedOrRemovedSymlinks bool
101-
deferredClose bool
102-
pendingReload PendingReload
103-
dirtyFilePath tspath.Path
96+
mu sync.Mutex
97+
initialLoadPending bool
98+
dirty bool
99+
version int
100+
deferredClose bool
101+
pendingReload PendingReload
102+
dirtyFilePath tspath.Path
104103

105104
comparePathsOptions tspath.ComparePathsOptions
106105
currentDirectory string
@@ -114,7 +113,6 @@ type Project struct {
114113
rootFileNames *collections.OrderedMap[tspath.Path, string]
115114
compilerOptions *core.CompilerOptions
116115
parsedCommandLine *tsoptions.ParsedCommandLine
117-
programMu sync.Mutex
118116
program *compiler.Program
119117
checkerPool *checkerPool
120118

@@ -211,7 +209,7 @@ func (p *Project) GetSourceFile(fileName string, path tspath.Path, languageVersi
211209

212210
// Updates the program if needed.
213211
func (p *Project) GetProgram() *compiler.Program {
214-
p.updateIfDirty()
212+
p.updateGraph()
215213
return p.program
216214
}
217215

@@ -372,9 +370,9 @@ func (p *Project) getScriptKind(fileName string) core.ScriptKind {
372370
return core.GetScriptKindFromFileName(fileName)
373371
}
374372

375-
func (p *Project) markFileAsDirty(path tspath.Path) {
376-
p.dirtyStateMu.Lock()
377-
defer p.dirtyStateMu.Unlock()
373+
func (p *Project) MarkFileAsDirty(path tspath.Path) {
374+
p.mu.Lock()
375+
defer p.mu.Unlock()
378376
if !p.dirty {
379377
p.dirty = true
380378
p.dirtyFilePath = path
@@ -385,69 +383,57 @@ func (p *Project) markFileAsDirty(path tspath.Path) {
385383
}
386384

387385
func (p *Project) markAsDirty() {
388-
p.dirtyStateMu.Lock()
389-
defer p.dirtyStateMu.Unlock()
386+
p.mu.Lock()
387+
defer p.mu.Unlock()
388+
p.markAsDirtyLocked()
389+
}
390+
391+
func (p *Project) markAsDirtyLocked() {
390392
p.dirtyFilePath = ""
391393
if !p.dirty {
392394
p.dirty = true
393395
p.version++
394396
}
395397
}
396398

397-
// updateIfDirty returns true if the project was updated.
398-
func (p *Project) updateIfDirty() bool {
399-
// !!! p.invalidateResolutionsOfFailedLookupLocations()
400-
return p.dirty && p.updateGraph()
401-
}
402-
403-
func (p *Project) onFileAddedOrRemoved(isSymlink bool) {
404-
p.dirtyStateMu.Lock()
405-
defer p.dirtyStateMu.Unlock()
406-
p.hasAddedOrRemovedFiles = true
407-
if isSymlink {
408-
p.hasAddedOrRemovedSymlinks = true
409-
}
410-
}
411-
412399
// updateGraph updates the set of files that contribute to the project.
413400
// Returns true if the set of files in has changed. NOTE: this is the
414401
// opposite of the return value in Strada, which was frequently inverted,
415402
// as in `updateProjectIfDirty()`.
416403
func (p *Project) updateGraph() bool {
417-
p.programMu.Lock()
418-
defer p.programMu.Unlock()
404+
p.mu.Lock()
405+
defer p.mu.Unlock()
406+
419407
if !p.dirty {
420408
return false
421409
}
422410

411+
start := time.Now()
423412
p.log("Starting updateGraph: Project: " + p.name)
413+
var writeFileNames bool
424414
oldProgram := p.program
425-
hasAddedOrRemovedFiles := p.hasAddedOrRemovedFiles
426415
p.initialLoadPending = false
427416

428417
if p.kind == KindConfigured && p.pendingReload != PendingReloadNone {
429418
switch p.pendingReload {
430419
case PendingReloadFileNames:
431420
p.parsedCommandLine = tsoptions.ReloadFileNamesOfParsedCommandLine(p.parsedCommandLine, p.host.FS())
432-
p.setRootFiles(p.parsedCommandLine.FileNames())
421+
writeFileNames = p.setRootFiles(p.parsedCommandLine.FileNames())
433422
case PendingReloadFull:
434-
if err := p.LoadConfig(); err != nil {
423+
if err := p.loadConfig(); err != nil {
435424
panic(fmt.Sprintf("failed to reload config: %v", err))
436425
}
437426
}
438427
p.pendingReload = PendingReloadNone
439428
}
440429

441-
p.hasAddedOrRemovedFiles = false
442-
p.hasAddedOrRemovedSymlinks = false
443430
oldProgramReused := p.updateProgram()
444431
p.dirty = false
445432
p.dirtyFilePath = ""
446-
p.log(fmt.Sprintf("Finishing updateGraph: Project: %s version: %d", p.name, p.version))
447-
if hasAddedOrRemovedFiles {
433+
if writeFileNames {
448434
p.log(p.print(true /*writeFileNames*/, true /*writeFileExplanation*/, false /*writeFileVersionAndText*/))
449435
} else if p.program != oldProgram {
450-
p.log("Different program with same set of files")
436+
p.log("Different program with same set of root files")
451437
}
452438
if !oldProgramReused {
453439
if oldProgram != nil {
@@ -461,6 +447,7 @@ func (p *Project) updateGraph() bool {
461447
// but in Strada we throttle, so at least sometimes this should be considered top-level?
462448
p.updateWatchers(context.TODO())
463449
}
450+
p.log(fmt.Sprintf("Finishing updateGraph: Project: %s version: %d in %s", p.name, p.version, time.Since(start)))
464451
return true
465452
}
466453

@@ -509,6 +496,13 @@ func (p *Project) isRoot(info *ScriptInfo) bool {
509496
return p.rootFileNames.Has(info.path)
510497
}
511498

499+
func (p *Project) RemoveFile(info *ScriptInfo, fileExists bool, detachFromProject bool) {
500+
p.mu.Lock()
501+
defer p.mu.Unlock()
502+
p.removeFile(info, fileExists, detachFromProject)
503+
p.markAsDirtyLocked()
504+
}
505+
512506
func (p *Project) removeFile(info *ScriptInfo, fileExists bool, detachFromProject bool) {
513507
if p.isRoot(info) {
514508
switch p.kind {
@@ -530,7 +524,13 @@ func (p *Project) removeFile(info *ScriptInfo, fileExists bool, detachFromProjec
530524
if detachFromProject {
531525
info.detachFromProject(p)
532526
}
533-
p.markAsDirty()
527+
}
528+
529+
func (p *Project) AddRoot(info *ScriptInfo) {
530+
p.mu.Lock()
531+
defer p.mu.Unlock()
532+
p.addRoot(info)
533+
p.markAsDirtyLocked()
534534
}
535535

536536
func (p *Project) addRoot(info *ScriptInfo) {
@@ -544,10 +544,17 @@ func (p *Project) addRoot(info *ScriptInfo) {
544544
}
545545
p.rootFileNames.Set(info.path, info.fileName)
546546
info.attachToProject(p)
547-
p.markAsDirty()
548547
}
549548

550549
func (p *Project) LoadConfig() error {
550+
if err := p.loadConfig(); err != nil {
551+
return err
552+
}
553+
p.markAsDirty()
554+
return nil
555+
}
556+
557+
func (p *Project) loadConfig() error {
551558
if p.kind != KindConfigured {
552559
panic("loadConfig called on non-configured project")
553560
}
@@ -582,12 +589,12 @@ func (p *Project) LoadConfig() error {
582589
p.compilerOptions = &core.CompilerOptions{}
583590
return fmt.Errorf("could not read file %q", p.configFileName)
584591
}
585-
586-
p.markAsDirty()
587592
return nil
588593
}
589594

590-
func (p *Project) setRootFiles(rootFileNames []string) {
595+
// setRootFiles returns true if the set of root files has changed.
596+
func (p *Project) setRootFiles(rootFileNames []string) bool {
597+
var hasChanged bool
591598
newRootScriptInfos := make(map[tspath.Path]struct{}, len(rootFileNames))
592599
for _, file := range rootFileNames {
593600
scriptKind := p.getScriptKind(file)
@@ -597,6 +604,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
597604
scriptInfo := p.host.GetOrCreateScriptInfoForFile(file, path, scriptKind)
598605
newRootScriptInfos[path] = struct{}{}
599606
isAlreadyRoot := p.rootFileNames.Has(path)
607+
hasChanged = hasChanged || !isAlreadyRoot
600608

601609
if !isAlreadyRoot && scriptInfo != nil {
602610
p.addRoot(scriptInfo)
@@ -610,6 +618,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
610618
}
611619

612620
if p.rootFileNames.Size() > len(rootFileNames) {
621+
hasChanged = true
613622
for root := range p.rootFileNames.Keys() {
614623
if _, ok := newRootScriptInfos[root]; !ok {
615624
if info := p.host.GetScriptInfoByPath(root); info != nil {
@@ -620,6 +629,7 @@ func (p *Project) setRootFiles(rootFileNames []string) {
620629
}
621630
}
622631
}
632+
return hasChanged
623633
}
624634

625635
func (p *Project) clearSourceMapperCache() {

‎internal/project/scriptinfo.go

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func (s *ScriptInfo) setText(newText string) {
110110

111111
func (s *ScriptInfo) markContainingProjectsAsDirty() {
112112
for _, project := range s.containingProjects {
113-
project.markFileAsDirty(s.path)
113+
project.MarkFileAsDirty(s.path)
114114
}
115115
}
116116

@@ -122,7 +122,6 @@ func (s *ScriptInfo) attachToProject(project *Project) bool {
122122
if project.compilerOptions.PreserveSymlinks != core.TSTrue {
123123
s.ensureRealpath(project.FS())
124124
}
125-
project.onFileAddedOrRemoved(s.isSymlink())
126125
return true
127126
}
128127
return false
@@ -132,11 +131,6 @@ func (s *ScriptInfo) isAttached(project *Project) bool {
132131
return slices.Contains(s.containingProjects, project)
133132
}
134133

135-
func (s *ScriptInfo) isSymlink() bool {
136-
// !!!
137-
return false
138-
}
139-
140134
func (s *ScriptInfo) isOrphan() bool {
141135
if s.deferredDelete {
142136
return true
@@ -181,15 +175,13 @@ func (s *ScriptInfo) detachAllProjects() {
181175
// if (isConfiguredProject(p)) {
182176
// p.getCachedDirectoryStructureHost().addOrDeleteFile(this.fileName, this.path, FileWatcherEventKind.Deleted);
183177
// }
184-
project.removeFile(s, false /*fileExists*/, false /*detachFromProject*/)
185-
project.onFileAddedOrRemoved(s.isSymlink())
178+
project.RemoveFile(s, false /*fileExists*/, false /*detachFromProject*/)
186179
}
187180
s.containingProjects = nil
188181
}
189182

190183
func (s *ScriptInfo) detachFromProject(project *Project) {
191184
if index := slices.Index(s.containingProjects, project); index != -1 {
192-
s.containingProjects[index].onFileAddedOrRemoved(s.isSymlink())
193185
s.containingProjects = slices.Delete(s.containingProjects, index, index+1)
194186
}
195187
}

‎internal/project/service.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ func (s *Service) CloseFile(fileName string) {
211211
info.close(fileExists)
212212
for _, project := range info.containingProjects {
213213
if project.kind == KindInferred && project.isRoot(info) {
214-
project.removeFile(info, fileExists, true /*detachFromProject*/)
214+
project.RemoveFile(info, fileExists, true /*detachFromProject*/)
215215
}
216216
}
217217
delete(s.openFiles, info.path)
@@ -309,17 +309,17 @@ func (s *Service) onConfigFileChanged(project *Project, changeKind lsproto.FileC
309309
project.pendingReload = PendingReloadFull
310310
project.markAsDirty()
311311
}
312-
project.updateIfDirty()
312+
project.updateGraph()
313313
return nil
314314
}
315315

316316
func (s *Service) ensureProjectStructureUpToDate() {
317317
var hasChanges bool
318318
for _, project := range s.configuredProjects {
319-
hasChanges = project.updateIfDirty() || hasChanges
319+
hasChanges = project.updateGraph() || hasChanges
320320
}
321321
for _, project := range s.inferredProjects {
322-
hasChanges = project.updateIfDirty() || hasChanges
322+
hasChanges = project.updateGraph() || hasChanges
323323
}
324324
if hasChanges {
325325
s.ensureProjectForOpenFiles()
@@ -342,7 +342,7 @@ func (s *Service) ensureProjectForOpenFiles() {
342342
}
343343
}
344344
for _, project := range s.inferredProjects {
345-
project.updateIfDirty()
345+
project.updateGraph()
346346
}
347347

348348
s.Log("After ensureProjectForOpenFiles:")
@@ -570,7 +570,7 @@ func (s *Service) assignProjectToOpenedScriptInfo(info *ScriptInfo) assignProjec
570570
// result.configFileErrors = project.getAllProjectErrors()
571571
}
572572
for _, project := range info.containingProjects {
573-
project.updateIfDirty()
573+
project.updateGraph()
574574
}
575575
if info.isOrphan() {
576576
// !!!
@@ -598,7 +598,7 @@ func (s *Service) assignOrphanScriptInfoToInferredProject(info *ScriptInfo, proj
598598
project = s.getOrCreateUnrootedInferredProject()
599599
}
600600

601-
project.addRoot(info)
601+
project.AddRoot(info)
602602
project.updateGraph()
603603
// !!! old code ensures that scriptInfo is only part of one project
604604
}

0 commit comments

Comments
 (0)