Skip to content

Commit 187febd

Browse files
sheetalkamatsheetalkamat
authored
Merge pull request #16494 from Microsoft/dontCreateScriptInfosWithFileExistsAndReadFile
Fixes the memory leak because of project and its corresponding script info even after project is removed
2 parents a3f39ec + 428bc68 commit 187febd

File tree

5 files changed

+87
-32
lines changed

5 files changed

+87
-32
lines changed

‎src/harness/unittests/tsserverProjectSystem.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2204,7 +2204,8 @@ namespace ts.projectSystem {
22042204
projectService.closeClientFile(f1.path);
22052205
projectService.checkNumberOfProjects({});
22062206

2207-
for (const f of [f2, f3]) {
2207+
for (const f of [f1, f2, f3]) {
2208+
// There shouldnt be any script info as we closed the file that resulted in creation of it
22082209
const scriptInfo = projectService.getScriptInfoForNormalizedPath(server.toNormalizedPath(f.path));
22092210
assert.equal(scriptInfo.containingProjects.length, 0, `expect 0 containing projects for '${f.path}'`);
22102211
}

‎src/server/editorServices.ts

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -565,10 +565,17 @@ namespace ts.server {
565565
}
566566
else {
567567
if (info && (!info.isScriptOpen())) {
568-
// file has been changed which might affect the set of referenced files in projects that include
569-
// this file and set of inferred projects
570-
info.reloadFromFile();
571-
this.updateProjectGraphs(info.containingProjects);
568+
if (info.containingProjects.length === 0) {
569+
// Orphan script info, remove it as we can always reload it on next open
570+
info.stopWatcher();
571+
this.filenameToScriptInfo.remove(info.path);
572+
}
573+
else {
574+
// file has been changed which might affect the set of referenced files in projects that include
575+
// this file and set of inferred projects
576+
info.reloadFromFile();
577+
this.updateProjectGraphs(info.containingProjects);
578+
}
572579
}
573580
}
574581
}
@@ -829,10 +836,29 @@ namespace ts.server {
829836
this.assignScriptInfoToInferredProjectIfNecessary(f, /*addToListOfOpenFiles*/ false);
830837
}
831838
}
839+
840+
// Cleanup script infos that arent part of any project is postponed to
841+
// next file open so that if file from same project is opened we wont end up creating same script infos
832842
}
833-
if (info.containingProjects.length === 0) {
834-
// if there are not projects that include this script info - delete it
835-
this.filenameToScriptInfo.remove(info.path);
843+
844+
// If the current info is being just closed - add the watcher file to track changes
845+
// But if file was deleted, handle that part
846+
if (this.host.fileExists(info.fileName)) {
847+
this.watchClosedScriptInfo(info);
848+
}
849+
else {
850+
this.handleDeletedFile(info);
851+
}
852+
}
853+
854+
private deleteOrphanScriptInfoNotInAnyProject() {
855+
for (const path of this.filenameToScriptInfo.getKeys()) {
856+
const info = this.filenameToScriptInfo.get(path);
857+
if (!info.isScriptOpen() && info.containingProjects.length === 0) {
858+
// if there are not projects that include this script info - delete it
859+
info.stopWatcher();
860+
this.filenameToScriptInfo.remove(info.path);
861+
}
836862
}
837863
}
838864

@@ -1303,6 +1329,14 @@ namespace ts.server {
13031329
return this.getScriptInfoForNormalizedPath(toNormalizedPath(uncheckedFileName));
13041330
}
13051331

1332+
watchClosedScriptInfo(info: ScriptInfo) {
1333+
// do not watch files with mixed content - server doesn't know how to interpret it
1334+
if (!info.hasMixedContent) {
1335+
const { fileName } = info;
1336+
info.setWatcher(this.host.watchFile(fileName, _ => this.onSourceFileChanged(fileName)));
1337+
}
1338+
}
1339+
13061340
getOrCreateScriptInfoForNormalizedPath(fileName: NormalizedPath, openedByClient: boolean, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean) {
13071341
let info = this.getScriptInfoForNormalizedPath(fileName);
13081342
if (!info) {
@@ -1318,15 +1352,13 @@ namespace ts.server {
13181352
}
13191353
}
13201354
else {
1321-
// do not watch files with mixed content - server doesn't know how to interpret it
1322-
if (!hasMixedContent) {
1323-
info.setWatcher(this.host.watchFile(fileName, _ => this.onSourceFileChanged(fileName)));
1324-
}
1355+
this.watchClosedScriptInfo(info);
13251356
}
13261357
}
13271358
}
13281359
if (info) {
13291360
if (openedByClient && !info.isScriptOpen()) {
1361+
info.stopWatcher();
13301362
info.open(fileContent);
13311363
if (hasMixedContent) {
13321364
info.registerFileUpdate();
@@ -1421,6 +1453,7 @@ namespace ts.server {
14211453
for (const p of this.inferredProjects) {
14221454
p.updateGraph();
14231455
}
1456+
14241457
this.printProjects();
14251458
}
14261459

@@ -1454,6 +1487,11 @@ namespace ts.server {
14541487
// at this point if file is the part of some configured/external project then this project should be created
14551488
const info = this.getOrCreateScriptInfoForNormalizedPath(fileName, /*openedByClient*/ true, fileContent, scriptKind, hasMixedContent);
14561489
this.assignScriptInfoToInferredProjectIfNecessary(info, /*addToListOfOpenFiles*/ true);
1490+
// Delete the orphan files here because there might be orphan script infos (which are not part of project)
1491+
// when some file/s were closed which resulted in project removal.
1492+
// It was then postponed to cleanup these script infos so that they can be reused if
1493+
// the file from that old project is reopened because of opening file from here.
1494+
this.deleteOrphanScriptInfoNotInAnyProject();
14571495
this.printProjects();
14581496
return { configFileName, configFileErrors };
14591497
}

‎src/server/lsHost.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,11 @@ namespace ts.server {
1111

1212
private filesWithChangedSetOfUnresolvedImports: Path[];
1313

14-
private readonly resolveModuleName: typeof resolveModuleName;
14+
private resolveModuleName: typeof resolveModuleName;
1515
readonly trace: (s: string) => void;
1616
readonly realpath?: (path: string) => string;
1717

18-
constructor(private readonly host: ServerHost, private readonly project: Project, private readonly cancellationToken: HostCancellationToken) {
18+
constructor(private readonly host: ServerHost, private project: Project, private readonly cancellationToken: HostCancellationToken) {
1919
this.cancellationToken = new ThrottledCancellationToken(cancellationToken, project.projectService.throttleWaitMilliseconds);
2020
this.getCanonicalFileName = ts.createGetCanonicalFileName(this.host.useCaseSensitiveFileNames);
2121

@@ -47,6 +47,11 @@ namespace ts.server {
4747
}
4848
}
4949

50+
dispose() {
51+
this.project = undefined;
52+
this.resolveModuleName = undefined;
53+
}
54+
5055
public startRecordingFilesWithChangedResolutions() {
5156
this.filesWithChangedSetOfUnresolvedImports = [];
5257
}
@@ -238,4 +243,4 @@ namespace ts.server {
238243
this.compilationSettings = opt;
239244
}
240245
}
241-
}
246+
}

‎src/server/project.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ namespace ts.server {
116116

117117
public languageServiceEnabled = true;
118118

119-
protected readonly lsHost: LSHost;
119+
protected lsHost: LSHost;
120120

121121
builder: Builder;
122122
/**
@@ -297,9 +297,15 @@ namespace ts.server {
297297
this.rootFiles = undefined;
298298
this.rootFilesMap = undefined;
299299
this.program = undefined;
300+
this.builder = undefined;
301+
this.cachedUnresolvedImportsPerFile = undefined;
302+
this.projectErrors = undefined;
303+
this.lsHost.dispose();
304+
this.lsHost = undefined;
300305

301306
// signal language service to release source files acquired from document registry
302307
this.languageService.dispose();
308+
this.languageService = undefined;
303309
}
304310

305311
getCompilerOptions() {
@@ -1043,6 +1049,7 @@ namespace ts.server {
10431049

10441050
if (this.projectFileWatcher) {
10451051
this.projectFileWatcher.close();
1052+
this.projectFileWatcher = undefined;
10461053
}
10471054

10481055
if (this.typeRootsWatchers) {
@@ -1132,4 +1139,4 @@ namespace ts.server {
11321139
this.typeAcquisition = newTypeAcquisition;
11331140
}
11341141
}
1135-
}
1142+
}

‎src/services/services.ts

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -822,7 +822,7 @@ namespace ts {
822822
private _compilationSettings: CompilerOptions;
823823
private currentDirectory: string;
824824

825-
constructor(private host: LanguageServiceHost, private getCanonicalFileName: (fileName: string) => string) {
825+
constructor(private host: LanguageServiceHost, getCanonicalFileName: (fileName: string) => string) {
826826
// script id => script index
827827
this.currentDirectory = host.getCurrentDirectory();
828828
this.fileNameToEntry = createFileMap<HostFileInformation>();
@@ -857,22 +857,17 @@ namespace ts {
857857
return entry;
858858
}
859859

860-
private getEntry(path: Path): HostFileInformation {
860+
public getEntryByPath(path: Path): HostFileInformation {
861861
return this.fileNameToEntry.get(path);
862862
}
863863

864-
private contains(path: Path): boolean {
864+
public containsEntryByPath(path: Path): boolean {
865865
return this.fileNameToEntry.contains(path);
866866
}
867867

868-
public getOrCreateEntry(fileName: string): HostFileInformation {
869-
const path = toPath(fileName, this.currentDirectory, this.getCanonicalFileName);
870-
return this.getOrCreateEntryByPath(fileName, path);
871-
}
872-
873868
public getOrCreateEntryByPath(fileName: string, path: Path): HostFileInformation {
874-
return this.contains(path)
875-
? this.getEntry(path)
869+
return this.containsEntryByPath(path)
870+
? this.getEntryByPath(path)
876871
: this.createEntry(fileName, path);
877872
}
878873

@@ -889,12 +884,12 @@ namespace ts {
889884
}
890885

891886
public getVersion(path: Path): string {
892-
const file = this.getEntry(path);
887+
const file = this.getEntryByPath(path);
893888
return file && file.version;
894889
}
895890

896891
public getScriptSnapshot(path: Path): IScriptSnapshot {
897-
const file = this.getEntry(path);
892+
const file = this.getEntryByPath(path);
898893
return file && file.scriptSnapshot;
899894
}
900895
}
@@ -1159,12 +1154,19 @@ namespace ts {
11591154
getCurrentDirectory: () => currentDirectory,
11601155
fileExists: (fileName): boolean => {
11611156
// stub missing host functionality
1162-
return hostCache.getOrCreateEntry(fileName) !== undefined;
1157+
const path = toPath(fileName, currentDirectory, getCanonicalFileName);
1158+
return hostCache.containsEntryByPath(path) ?
1159+
!!hostCache.getEntryByPath(path) :
1160+
(host.fileExists && host.fileExists(fileName));
11631161
},
11641162
readFile: (fileName): string => {
11651163
// stub missing host functionality
1166-
const entry = hostCache.getOrCreateEntry(fileName);
1167-
return entry && entry.scriptSnapshot.getText(0, entry.scriptSnapshot.getLength());
1164+
const path = toPath(fileName, currentDirectory, getCanonicalFileName);
1165+
if (hostCache.containsEntryByPath(path)) {
1166+
const entry = hostCache.getEntryByPath(path);
1167+
return entry && entry.scriptSnapshot.getText(0, entry.scriptSnapshot.getLength());
1168+
}
1169+
return host.readFile && host.readFile(fileName);
11681170
},
11691171
directoryExists: directoryName => {
11701172
return directoryProbablyExists(directoryName, host);
@@ -1316,7 +1318,9 @@ namespace ts {
13161318
if (program) {
13171319
forEach(program.getSourceFiles(), f =>
13181320
documentRegistry.releaseDocument(f.fileName, program.getCompilerOptions()));
1321+
program = undefined;
13191322
}
1323+
host = undefined;
13201324
}
13211325

13221326
/// Diagnostics

0 commit comments

Comments
 (0)