Skip to content

Protect watcher from double close #49990

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 22, 2022
Merged

Protect watcher from double close #49990

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/compiler/sys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,14 +1088,16 @@ namespace ts {
lastDirectoryPart = lastDirectoryPartWithDirectorySeparator.slice(directorySeparator.length);
}
/** Watcher for the file system entry depending on whether it is missing or present */
let watcher = !fileSystemEntryExists(fileOrDirectory, entryKind) ?
let watcher: FileWatcher | undefined = !fileSystemEntryExists(fileOrDirectory, entryKind) ?
watchMissingFileSystemEntry() :
watchPresentFileSystemEntry();
return {
close: () => {
// Close the watcher (either existing file system entry watcher or missing file system entry watcher)
watcher.close();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What i am saying is that this should be watcher!.close() closing same watcher multiple times is potentially issue somewhere else.

Watcher is set to undefined because we want to update them if its not closed look for updateWatcher right below this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly what the code is today, though. We have let watcher: FileWatcher = ....

If we leave this code as it is now, it will crash if close is called, because it sets the watcher to undefined.

This PR doesn't change the fact that the watcher can be undefined, it only declares that watcher can be undefined (which makes it clear that watcher.close() is a type error), then ignores all closes but the first by skipping if it's already been set to undefined. watcher is still set to undefined so updateWatcher will behave identically and not perform any updates if the watcher has been closed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, my suggestion of

make it always required and remove the undefined! and allow close to proceed if it's called a second time

Doesn't work, of course, if we are trying to avoid updateWatcher from creating a new watcher even if it's closed.

So, we can either ignore the double close (this PR), or I guess we can do a debug assert and figure out who is closing this a second time and shouldn't be (which is the thing I was unsure was correct, given there are watchers out there that are refcounted and intend to be closed more than once).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given there are watchers out there that are refcounted and intend to be closed more than once).

This is not one of the watcher thats ref counted ones close over physical watcher and are suppose to close only once when ref count is 0

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, we should do what this PR does then, and ignore a second close?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave more thought to this.. I think Debug.assert would be ideal but given its more likely to cause it in case of other exceptions i think your fix is good. Sorry for going back and forth on this one..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, just wanted to ensure I understood the right thing here.

I did try and add some debug asserts to every close implementation I could find (considering ref counting too), but didn't actually hit any asserts either. I guess that's what happens without a repro.

watcher = undefined!;
if (watcher) {
watcher.close();
watcher = undefined;
}
}
};

Expand Down