Skip to content
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

Cleanup old file/folder watch callbacks #3536

Open
bradzacher opened this issue Jun 16, 2021 · 1 comment
Open

Cleanup old file/folder watch callbacks #3536

bradzacher opened this issue Jun 16, 2021 · 1 comment
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance

Comments

@bradzacher
Copy link
Member

As part of supporting IDE runs we use TS "watch programs" to allow efficient recomputation of the TS program as the IDE sends us updated code for files.

However, as we also need to simultaneously support CLI runs we cannot allow TS to actually attach file watchers to the filesystem. File watchers count as open pipes - which will cause NodeJS to prevent ESLint from ever exiting (making it seem like ESLint has "hung").

So in order for us to "get the best of both worlds", we take the callback functions that TS uses to be alerted of filesystem changes and we store them in a map of sets (keyed by the file path).

/**
* Maps file/folder paths to their set of corresponding watch callbacks
* There may be more than one per file/folder if a file/folder is shared between projects
*/
const fileWatchCallbackTrackingMap = new Map<
CanonicalPath,
Set<ts.FileWatcherCallback>
>();
const folderWatchCallbackTrackingMap = new Map<
CanonicalPath,
Set<ts.FileWatcherCallback>
>();

watchCompilerHost.watchFile = saveWatchCallback(fileWatchCallbackTrackingMap);
watchCompilerHost.watchDirectory = saveWatchCallback(
folderWatchCallbackTrackingMap,
);

function saveWatchCallback(
trackingMap: Map<string, Set<ts.FileWatcherCallback>>,
) {
return (
fileName: string,
callback: ts.FileWatcherCallback,
): ts.FileWatcher => {
const normalizedFileName = getCanonicalFileName(fileName);
const watchers = ((): Set<ts.FileWatcherCallback> => {
let watchers = trackingMap.get(normalizedFileName);
if (!watchers) {
watchers = new Set();
trackingMap.set(normalizedFileName, watchers);
}
return watchers;
})();
watchers.add(callback);
return {
close: (): void => {
watchers.delete(callback);
},
};
};
}

Looking at TS's source code...

It has one place where it passes in a constant function pointer in to watchFile:

And many places where it passes arrow functions in to watchFile/watchDirectory:

As we (and the TS program) do not have any "cleanup" step - it means we never clear these caches. We do not know when or if we can remove functions from these caches. This means we can accumulate these callbacks indefinitely.

They're just itty bitty function references - what's the problem?!? The issue is that functions have a closure which stores variables. In some of these cases the closure contains a reference to a SourceFile. Which means that Node cannot GC the SourceFile (or anything else in the closure for that matter) unless we free our function reference.

This is a problem as SourceFiles aren't the lightest data structure - so unbounded accumulation of them can cause an OOM. An extreme example of this is #3533 - wherein they had a bad config file, meaning we would throw away and recreate the watch host for each file without cleaning up its watch functions.

I don't know if this actually impacts "good" project setups - but we should really profile this and figure out what sort of memory usage this causes for different usecases (big project, small project, many projects). I don't have a good solution for this either. We can't use WeakRef because we are the sole owner of the function reference.. so we would need another mechanism to clean up.

cc @uniqueiniquity for the TS side of things

@bradzacher bradzacher added bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree labels Jun 16, 2021
@JoshuaKGoldberg JoshuaKGoldberg added performance Issues regarding performance blocked by another issue Issues which are not ready because another issue needs to be resolved first labels Feb 24, 2022
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Feb 25, 2022

Marking as blocked on #4183 -> #6020.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first bug Something isn't working package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance
Projects
None yet
Development

No branches or pull requests

2 participants