From 07ba90659404830f735f819b645476954c3c5d9a Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 9 Oct 2017 14:25:48 -0700 Subject: [PATCH 1/4] Handle the case when finishCachingPerDirectoryResolution is not called because of exception Fixes #18975 --- src/compiler/resolutionCache.ts | 10 ++++++++-- src/compiler/watch.ts | 10 +++++----- src/server/project.ts | 7 +++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index aecc6989891..680ed98a84a 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -141,7 +141,10 @@ namespace ts { resolvedModuleNames.clear(); resolvedTypeReferenceDirectives.clear(); allFilesHaveInvalidatedResolution = false; - Debug.assert(perDirectoryResolvedModuleNames.size === 0 && perDirectoryResolvedTypeReferenceDirectives.size === 0); + // perDirectoryResolvedModuleNames and perDirectoryResolvedTypeReferenceDirectives could be non empty if there was exception during program update + // (between startCachingPerDirectoryResolution and finishCachingPerDirectoryResolution) + perDirectoryResolvedModuleNames.clear(); + perDirectoryResolvedTypeReferenceDirectives.clear(); } function startRecordingFilesWithChangedResolutions() { @@ -166,7 +169,10 @@ namespace ts { } function startCachingPerDirectoryResolution() { - Debug.assert(perDirectoryResolvedModuleNames.size === 0 && perDirectoryResolvedTypeReferenceDirectives.size === 0); + // perDirectoryResolvedModuleNames and perDirectoryResolvedTypeReferenceDirectives could be non empty if there was exception during program update + // (between startCachingPerDirectoryResolution and finishCachingPerDirectoryResolution) + perDirectoryResolvedModuleNames.clear(); + perDirectoryResolvedTypeReferenceDirectives.clear(); } function finishCachingPerDirectoryResolution() { diff --git a/src/compiler/watch.ts b/src/compiler/watch.ts index 2ab3ccc5ce6..1ab80e659f4 100644 --- a/src/compiler/watch.ts +++ b/src/compiler/watch.ts @@ -322,6 +322,9 @@ namespace ts { if (hasChangedCompilerOptions) { newLine = getNewLineCharacter(compilerOptions, system); + if (changesAffectModuleResolution(program && program.getCompilerOptions(), compilerOptions)) { + resolutionCache.clear(); + } } const hasInvalidatedResolution = resolutionCache.createHasInvalidatedResolution(); @@ -329,14 +332,11 @@ namespace ts { return; } - if (hasChangedCompilerOptions && changesAffectModuleResolution(program && program.getCompilerOptions(), compilerOptions)) { - resolutionCache.clear(); - } - const needsUpdateInTypeRootWatch = hasChangedCompilerOptions || !program; - hasChangedCompilerOptions = false; beforeCompile(compilerOptions); // Compile the program + const needsUpdateInTypeRootWatch = hasChangedCompilerOptions || !program; + hasChangedCompilerOptions = false; resolutionCache.startCachingPerDirectoryResolution(); compilerHost.hasInvalidatedResolution = hasInvalidatedResolution; compilerHost.hasChangedAutomaticTypeDirectiveNames = hasChangedAutomaticTypeDirectiveNames; diff --git a/src/server/project.ts b/src/server/project.ts index ac18738027b..5132b82a804 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -229,8 +229,8 @@ namespace ts.server { this.realpath = path => host.realpath(path); } - this.languageService = createLanguageService(this, this.documentRegistry); this.resolutionCache = createResolutionCache(this, rootDirectoryForResolution); + this.languageService = createLanguageService(this, this.documentRegistry); if (!languageServiceEnabled) { this.disableLanguageService(); } @@ -732,7 +732,6 @@ namespace ts.server { */ updateGraph(): boolean { this.resolutionCache.startRecordingFilesWithChangedResolutions(); - this.hasInvalidatedResolution = this.resolutionCache.createHasInvalidatedResolution(); let hasChanges = this.updateGraphWorker(); @@ -795,6 +794,7 @@ namespace ts.server { this.writeLog(`Starting updateGraphWorker: Project: ${this.getProjectName()}`); const start = timestamp(); + this.hasInvalidatedResolution = this.resolutionCache.createHasInvalidatedResolution(); this.resolutionCache.startCachingPerDirectoryResolution(); this.program = this.languageService.getProgram(); this.resolutionCache.finishCachingPerDirectoryResolution(); @@ -1327,14 +1327,13 @@ namespace ts.server { } close() { - super.close(); - if (this.configFileWatcher) { this.configFileWatcher.close(); this.configFileWatcher = undefined; } this.stopWatchingWildCards(); + super.close(); } addOpenRef() { From 6887dbc75028f225d25ed91815de1719e5a1e101 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 9 Oct 2017 15:24:00 -0700 Subject: [PATCH 2/4] Assert if the script info that is attached to closed project is present Adds assertion to investigate #19003 and #18928 --- src/server/editorServices.ts | 3 +++ src/server/project.ts | 21 ++++++++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 2916fb60c57..40c196009e3 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -832,6 +832,9 @@ namespace ts.server { this.logger.info(`remove project: ${project.getRootFiles().toString()}`); project.close(); + if (Debug.shouldAssert(AssertionLevel.Normal)) { + this.filenameToScriptInfo.forEach(info => Debug.assert(!info.isAttached(project))); + } // Remove the project from pending project updates this.pendingProjectUpdates.delete(project.getProjectName()); diff --git a/src/server/project.ts b/src/server/project.ts index 5132b82a804..e8bfd7c1b75 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -497,12 +497,7 @@ namespace ts.server { if (this.program) { // if we have a program - release all files that are enlisted in program for (const f of this.program.getSourceFiles()) { - const info = this.projectService.getScriptInfo(f.fileName); - // We might not find the script info in case its not associated with the project any more - // and project graph was not updated (eg delayed update graph in case of files changed/deleted on the disk) - if (info) { - info.detachFromProject(this); - } + this.detachScriptInfo(f.fileName); } } if (!this.program || !this.languageServiceEnabled) { @@ -512,10 +507,13 @@ namespace ts.server { root.detachFromProject(this); } } + this.rootFiles = undefined; this.rootFilesMap = undefined; this.program = undefined; this.builder = undefined; + forEach(this.externalFiles, externalFile => this.detachScriptInfo(externalFile)); + this.externalFiles = undefined; this.resolutionCache.clear(); this.resolutionCache = undefined; this.cachedUnresolvedImportsPerFile = undefined; @@ -532,6 +530,15 @@ namespace ts.server { this.languageService = undefined; } + private detachScriptInfo(uncheckedFilename: string) { + const info = this.projectService.getScriptInfo(uncheckedFilename); + // We might not find the script info in case its not associated with the project any more + // and project graph was not updated (eg delayed update graph in case of files changed/deleted on the disk) + if (info) { + info.detachFromProject(this); + } + } + isClosed() { return this.rootFiles === undefined; } @@ -791,7 +798,7 @@ namespace ts.server { private updateGraphWorker() { const oldProgram = this.program; - + Debug.assert(!this.isClosed(), "Called update graph worker of closed project"); this.writeLog(`Starting updateGraphWorker: Project: ${this.getProjectName()}`); const start = timestamp(); this.hasInvalidatedResolution = this.resolutionCache.createHasInvalidatedResolution(); From 98d58d651747702f2f9252ad93125026a770c565 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 9 Oct 2017 20:12:53 -0700 Subject: [PATCH 3/4] Handle project close to release all the script infos held by the project --- src/server/project.ts | 24 +++++++++---------- .../reference/api/tsserverlibrary.d.ts | 1 + 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/server/project.ts b/src/server/project.ts index e8bfd7c1b75..db540ccfac2 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -495,25 +495,25 @@ namespace ts.server { close() { if (this.program) { - // if we have a program - release all files that are enlisted in program + // if we have a program - release all files that are enlisted in program but arent root + // The releasing of the roots happens later + // The project could have pending update remaining and hence the info could be in the files but not in program graph for (const f of this.program.getSourceFiles()) { - this.detachScriptInfo(f.fileName); + this.detachScriptInfoIfNotRoot(f.fileName); } } - if (!this.program || !this.languageServiceEnabled) { - // release all root files either if there is no program or language service is disabled. - // in the latter case set of root files can be larger than the set of files in program. - for (const root of this.rootFiles) { - root.detachFromProject(this); - } + // Release external files + forEach(this.externalFiles, externalFile => this.detachScriptInfoIfNotRoot(externalFile)); + // Always remove root files from the project + for (const root of this.rootFiles) { + root.detachFromProject(this); } this.rootFiles = undefined; this.rootFilesMap = undefined; + this.externalFiles = undefined; this.program = undefined; this.builder = undefined; - forEach(this.externalFiles, externalFile => this.detachScriptInfo(externalFile)); - this.externalFiles = undefined; this.resolutionCache.clear(); this.resolutionCache = undefined; this.cachedUnresolvedImportsPerFile = undefined; @@ -530,11 +530,11 @@ namespace ts.server { this.languageService = undefined; } - private detachScriptInfo(uncheckedFilename: string) { + private detachScriptInfoIfNotRoot(uncheckedFilename: string) { const info = this.projectService.getScriptInfo(uncheckedFilename); // We might not find the script info in case its not associated with the project any more // and project graph was not updated (eg delayed update graph in case of files changed/deleted on the disk) - if (info) { + if (info && !this.isRoot(info)) { info.detachFromProject(this); } } diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts index 7fe07813adc..0097ba24942 100644 --- a/tests/baselines/reference/api/tsserverlibrary.d.ts +++ b/tests/baselines/reference/api/tsserverlibrary.d.ts @@ -7136,6 +7136,7 @@ declare namespace ts.server { getExternalFiles(): SortedReadonlyArray; getSourceFile(path: Path): SourceFile; close(): void; + private detachScriptInfoIfNotRoot(uncheckedFilename); isClosed(): boolean; hasRoots(): boolean; getRootFiles(): NormalizedPath[]; From 55bbcff348b49d063efe91e5d22e957ddb358076 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 10 Oct 2017 16:36:09 -0700 Subject: [PATCH 4/4] Modify the changesAffectModuleResolution check --- src/compiler/watch.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/watch.ts b/src/compiler/watch.ts index 1ab80e659f4..4fc67c1cc8f 100644 --- a/src/compiler/watch.ts +++ b/src/compiler/watch.ts @@ -322,7 +322,7 @@ namespace ts { if (hasChangedCompilerOptions) { newLine = getNewLineCharacter(compilerOptions, system); - if (changesAffectModuleResolution(program && program.getCompilerOptions(), compilerOptions)) { + if (program && changesAffectModuleResolution(program.getCompilerOptions(), compilerOptions)) { resolutionCache.clear(); } }