-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add upper limit for the program size to prevent tsserver from crashing #7486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
4d3cff1
b155fa8
a3aa000
a6a466c
d4eb3b8
a839d93
225e3b4
c8e0b00
d7e1d34
cb46f16
74e3d7b
1b76294
5c9ce9e
94d44ad
d387050
4383f1a
e41b10b
3354436
550d912
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -696,6 +696,7 @@ namespace ts { | |
| let diagnosticsProducingTypeChecker: TypeChecker; | ||
| let noDiagnosticsTypeChecker: TypeChecker; | ||
| let classifiableNames: Map<string>; | ||
| let programSizeForNonTsFiles = 0; | ||
|
|
||
| let skipDefaultLib = options.noLib; | ||
| const supportedExtensions = getSupportedExtensions(options); | ||
|
|
@@ -742,7 +743,8 @@ namespace ts { | |
| (oldOptions.target !== options.target) || | ||
| (oldOptions.noLib !== options.noLib) || | ||
| (oldOptions.jsx !== options.jsx) || | ||
| (oldOptions.allowJs !== options.allowJs)) { | ||
| (oldOptions.allowJs !== options.allowJs) || | ||
| (oldOptions.disableSizeLimit !== options.disableSizeLimit)) { | ||
| oldProgram = undefined; | ||
| } | ||
| } | ||
|
|
@@ -1452,6 +1454,27 @@ namespace ts { | |
| return file; | ||
| } | ||
|
|
||
| if (!options.disableSizeLimit) { | ||
| if (programSizeForNonTsFiles === -1) { | ||
| return; | ||
| } | ||
| if (programSizeForNonTsFiles > maxProgramSizeForNonTsFiles) { | ||
| // If the program size limit was reached when processing a file, this file is | ||
| // likely in the problematic folder than contains too many files. | ||
| // Normally the folder is one level down from the commonSourceDirectory, for example, | ||
| // if the commonSourceDirectory is "/src/", and the last processed path was "/src/node_modules/a/b.js", | ||
| // we should show in the error message "/src/node_modules/". | ||
| const commonSourceDirectory = getCommonSourceDirectory(); | ||
| let rootLevelDirectory = path.substring(0, Math.max(commonSourceDirectory.length, path.indexOf(directorySeparator, commonSourceDirectory.length))); | ||
| if (rootLevelDirectory[rootLevelDirectory.length - 1] !== directorySeparator) { | ||
| rootLevelDirectory += directorySeparator; | ||
| } | ||
| programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Too_many_JavaScript_files_in_the_project_Consider_specifying_the_exclude_setting_in_project_configuration_to_limit_included_source_folders_The_likely_folder_to_exclude_is_0_To_disable_the_project_size_limit_set_the_disableSizeLimit_compiler_option_to_true, rootLevelDirectory)); | ||
| programSizeForNonTsFiles = -1; | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and this too. |
||
| } | ||
| } | ||
|
|
||
| // We haven't looked for this file, do so now and cache result | ||
| const file = host.getSourceFile(fileName, options.target, hostErrorMessage => { | ||
| if (refFile !== undefined && refPos !== undefined && refEnd !== undefined) { | ||
|
|
@@ -1463,6 +1486,10 @@ namespace ts { | |
| } | ||
| }); | ||
|
|
||
| if (!options.disableSizeLimit && file && file.text && !hasTypeScriptFileExtension(file.fileName)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like we do not need this in the program building, since we are doing it in the server. |
||
| programSizeForNonTsFiles += file.text.length; | ||
| } | ||
|
|
||
| filesByName.set(path, file); | ||
| if (file) { | ||
| file.wasReferenced = file.wasReferenced || isReference; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2484,6 +2484,10 @@ namespace ts { | |
| return forEach(supportedJavascriptExtensions, extension => fileExtensionIs(fileName, extension)); | ||
| } | ||
|
|
||
| export function hasTypeScriptFileExtension(fileName: string) { | ||
| return forEach(supportedTypeScriptExtensions, extension => fileExtensionIs(fileName, extension)); | ||
| } | ||
|
|
||
| /** | ||
| * Replace each instance of non-ascii characters by one, two, three, or four escape sequences | ||
| * representing the UTF-8 encoding of the character, and return the expanded char code list. | ||
|
|
@@ -2866,4 +2870,6 @@ namespace ts { | |
| export function isParameterPropertyDeclaration(node: ParameterDeclaration): boolean { | ||
| return node.flags & NodeFlags.AccessibilityModifier && node.parent.kind === SyntaxKind.Constructor && isClassLike(node.parent.parent); | ||
| } | ||
|
|
||
| export const maxProgramSizeForNonTsFiles = 20 * 1024 * 1024; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i would put this in program. and no need to export it. we only use it there.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now it is shared between the server and the program.ts though |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1217,10 +1217,43 @@ namespace ts.server { | |
| } | ||
| else { | ||
| const project = this.createProject(configFilename, projectOptions); | ||
| let programSizeForNonTsFiles = 0; | ||
|
|
||
| // As the project openning might not be complete if there are too many files, | ||
| // therefore to surface the diagnostics we need to make sure the given client file is opened. | ||
| if (clientFileName) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i thought we said we did not need this anymore? is this not the case?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file reading here doesn't have an upper limit, therefore it could be much more than 20M. In the repro case it is around 100M, and in testing I noticed server crashes without the limit. So I kept the limit here as well. |
||
| if (this.host.fileExists(clientFileName)) { | ||
| const currentClientFileInfo = this.openFile(clientFileName, /*openedByClient*/ true); | ||
| project.addRoot(currentClientFileInfo); | ||
| if (!hasTypeScriptFileExtension(currentClientFileInfo.fileName) && currentClientFileInfo.content) { | ||
| programSizeForNonTsFiles += currentClientFileInfo.content.length; | ||
| } | ||
| } | ||
| else { | ||
| return { errorMsg: "specified file " + clientFileName + " not found" }; | ||
| } | ||
| } | ||
|
|
||
| for (const rootFilename of projectOptions.files) { | ||
| if (rootFilename === clientFileName) { | ||
| continue; | ||
| } | ||
|
|
||
| if (this.host.fileExists(rootFilename)) { | ||
| const info = this.openFile(rootFilename, /*openedByClient*/ clientFileName == rootFilename); | ||
| project.addRoot(info); | ||
| if (projectOptions.compilerOptions.disableSizeLimit) { | ||
| const info = this.openFile(rootFilename, /*openedByClient*/ false); | ||
| project.addRoot(info); | ||
| } | ||
| else if (programSizeForNonTsFiles <= maxProgramSizeForNonTsFiles) { | ||
| const info = this.openFile(rootFilename, /*openedByClient*/ false); | ||
| project.addRoot(info); | ||
| if (!hasTypeScriptFileExtension(rootFilename)) { | ||
| programSizeForNonTsFiles += info.content.length; | ||
| } | ||
| } | ||
| else { | ||
| break; | ||
| } | ||
| } | ||
| else { | ||
| return { errorMsg: "specified file " + rootFilename + " not found" }; | ||
|
|
@@ -1251,7 +1284,10 @@ namespace ts.server { | |
| return error; | ||
| } | ||
| else { | ||
| const oldFileNames = project.compilerService.host.roots.map(info => info.fileName); | ||
| // if the project is too large, the root files might not have been all loaded if the total | ||
| // program size reached the upper limit. In that case project.projectOptions.files should | ||
| // be more precise. However this would only happen for configured project. | ||
| const oldFileNames = project.projectOptions ? project.projectOptions.files : project.compilerService.host.roots.map(info => info.fileName); | ||
| const newFileNames = projectOptions.files; | ||
| const fileNamesToRemove = oldFileNames.filter(f => newFileNames.indexOf(f) < 0); | ||
| const fileNamesToAdd = newFileNames.filter(f => oldFileNames.indexOf(f) < 0); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit.
return undefined;