Fix leak in File System Watcher

[tsmaeder]

Signed-off-by: Thomas Mäder [email protected]

What it does

The code was calling Promise.race() repeatedly while waiting for a file to come into existence. This added a PromiseReaction object each time, which never was collected.

Fixes #12001

Contributed on behalf of STMicroelectronics

How to test

Make sure file system watchers are still started correctly: in particular for

1. Open editors
2. Preferences, Launch configs, etc.
3. Workspace folders

Since the fix concerns file that don't exist in the beginning, it makes sense cases like when I add a new file .theia/launch.json and expect the launch configs to show up and changes to the file to be tracked.

Review checklist

• As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• As a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

The old code was very unpalatable and the new code looked much better, except it was missing the part where cancellation should reject the promise returned by start with WatcherDisposal that acts as some flag value.

I was also not understanding why the previous code caused a leak, and I still don't precisely understand the mechanism by which it happened. But I've found discussions about Promise.race causing leaks when used in certain patterns: nodejs/node#17469 (comment)

[tsmaeder]

I was also not understanding why the previous code caused a leak, and I still don't precisely understand the mechanism by which it happened.

When you say promiseA.then(promiseB), you need a pointer from promiseA to promiseB, so that when promiseA settles, you can start evaluating promiseB. This seems to be implemented via a PromiseReaction object in node (at least, that's what shows up in the heap dump). The same applies to race(): there needs to be some bookkeeping about which promise depende on which other promises. That bookkeeping consumes heap memory.

[paul-marechal]

My point is that some garbage collection should be happening and it's not. People on the Promise.race leak thread seem to suggest this is an issue with V8's Promise.race implementation. They went as far as to create a garbage-collector-friendly JS implementation...

What we need to remember when using Promise.race from now on is that nothing will get collected for as long as one of the promises that's part of the race hasn't completed. Even if we discard the race promise.

[paul-marechal]

Code LGTM, file watching still works for me and CI is not showing new failures.