-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 2 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 |
---|---|---|
|
@@ -748,7 +748,21 @@ namespace ts { | |
} | ||
|
||
if (!tryReuseStructureFromOldProgram()) { | ||
forEach(rootNames, name => processRootFile(name, /*isDefaultLib*/ false)); | ||
let programSize = 0; | ||
for (const name of rootNames) { | ||
const path = toPath(name, currentDirectory, getCanonicalFileName); | ||
if (programSize <= maxProgramSize) { | ||
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. We discussed a compiler option to disable the size check for cases where the project is legitimately large. I don't see that option in this code review. |
||
processRootFile(name, /*isDefaultLib*/ false); | ||
if (!hasTypeScriptFileExtension(name) && filesByName.get(path)) { | ||
programSize += filesByName.get(path).text.length; | ||
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. Looking in 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. Good catch! |
||
} | ||
} | ||
else { | ||
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Too_many_javascript_files_in_the_project_Consider_add_to_the_exclude_list_in_the_config_file)); | ||
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 were going to indicate the path to the file being processed when the limit was exceeded (as this is likely in whatever folder you should have been excluding)? |
||
break; | ||
} | ||
} | ||
|
||
// Do not process the default library if: | ||
// - The '--noLib' flag is used. | ||
// - A 'no-default-lib' reference comment is encountered in | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,7 @@ namespace ts { | |
watchDirectory?(path: string, callback: DirectoryWatcherCallback, recursive?: boolean): FileWatcher; | ||
}; | ||
|
||
export var sys: System = (function () { | ||
export var sys: System = (function() { | ||
|
||
function getWScriptSystem(): System { | ||
|
||
|
@@ -404,8 +404,8 @@ namespace ts { | |
const watchedFileSet = createWatchedFileSet(); | ||
|
||
function isNode4OrLater(): boolean { | ||
return parseInt(process.version.charAt(1)) >= 4; | ||
} | ||
return parseInt(process.version.charAt(1)) >= 4; | ||
} | ||
|
||
const platform: string = _os.platform(); | ||
// win32\win64 are case insensitive platforms, MacOS (darwin) by default is also case insensitive | ||
|
@@ -500,7 +500,10 @@ namespace ts { | |
for (const current of files) { | ||
const name = combinePaths(path, current); | ||
if (!contains(exclude, getCanonicalPath(name))) { | ||
const stat = _fs.statSync(name); | ||
// fs.statSync would throw an exception if the file is a symlink | ||
// whose linked file doesn't exist. fs.lstatSync would return a stat | ||
// object for the symlink file itself in this case | ||
const stat = _fs.lstatSync(name); | ||
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. This seems like a non-trivial change with a lot of nuance (for example, this means we won't traverse into directories now if they are created as symbolic links, as How is this part of setting a limit on file size when loading? Seems like this is a different issue and should be coded/evaluated separately. 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. This was an issue that caused tsserver to crash when editing the repro sample. The ember server generates tons of symlink files in the The alternative solution was to try |
||
if (stat.isFile()) { | ||
if (!extension || fileExtensionIs(name, extension)) { | ||
result.push(name); | ||
|
@@ -532,7 +535,7 @@ namespace ts { | |
// and https://github.com/Microsoft/TypeScript/issues/4643), therefore | ||
// if the current node.js version is newer than 4, use `fs.watch` instead. | ||
const watchSet = isNode4OrLater() ? watchedFileSet : pollingWatchedFileSet; | ||
const watchedFile = watchSet.addFile(filePath, callback); | ||
const watchedFile = watchSet.addFile(filePath, callback); | ||
return { | ||
close: () => watchSet.removeFile(watchedFile) | ||
}; | ||
|
@@ -562,7 +565,7 @@ namespace ts { | |
} | ||
); | ||
}, | ||
resolvePath: function (path: string): string { | ||
resolvePath: function(path: string): string { | ||
return _path.resolve(path); | ||
}, | ||
fileExists, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1217,13 +1217,35 @@ namespace ts.server { | |
} | ||
else { | ||
const project = this.createProject(configFilename, projectOptions); | ||
let programSize = 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) { | ||
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? 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. |
||
const currentClientFileInfo = this.openFile(clientFileName, /*openedByClient*/ true); | ||
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. Do you not need to worry about the |
||
project.addRoot(currentClientFileInfo); | ||
programSize += currentClientFileInfo.content.length; | ||
} | ||
|
||
for (const rootFilename of projectOptions.files) { | ||
if (this.host.fileExists(rootFilename)) { | ||
const info = this.openFile(rootFilename, /*openedByClient*/ clientFileName == rootFilename); | ||
project.addRoot(info); | ||
if (rootFilename === clientFileName) { | ||
continue; | ||
} | ||
|
||
if (programSize <= maxProgramSize) { | ||
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'm trying to understand why we need to repeat this check for the size of the content loaded here as well as in 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. Because tsserver opens up files before calling the |
||
if (this.host.fileExists(rootFilename)) { | ||
const info = this.openFile(rootFilename, /*openedByClient*/ false); | ||
project.addRoot(info); | ||
if (!hasTypeScriptFileExtension(rootFilename)) { | ||
programSize += info.content.length; | ||
} | ||
} | ||
else { | ||
return { errorMsg: "specified file " + rootFilename + " not found" }; | ||
} | ||
} | ||
else { | ||
return { errorMsg: "specified file " + rootFilename + " not found" }; | ||
break; | ||
} | ||
} | ||
project.finishGraph(); | ||
|
@@ -1251,7 +1273,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.
Grammar: "Consider adding to...". Or perhaps just reworded as "Use the 'exclude' setting in project configuration to limit included source folders".
Note: Do we predicate this message on the user not having listed the files to compile? If we are not scanning directories anyway (i.e. there is a list of files given), then this message would be misleading (as
exclude
can't be specified - at least until the globbing change comes in).Also, capitalize as "JavaScript", not "javascript", and don't use backticks. This isn't markdown :-p
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.
I agree that we should mention both cases, with or without the "exclude" list. Does the following sound better:
"Too many JavaScript files in the project. Use an exact 'files' list, or use the 'exclude' setting in project configuration to limit included source folders."
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.
I like it :-)