Protect watcher from double close #49990
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
jakebailey
assigned andrewbranch and jakebailey and unassigned jakebailey and andrewbranch
There was a problem hiding this comment.
I am not confident in if we should take this or not..
Normally this exception is either because the state is messed up because of other exception (in which case this fix is ok as it shuts down the noise) but there could be some legitimate issues with watcher ref counting etc that we might not be able to detect easily. So i am more on the side of not taking this fix rather than taking but not totally against.
|
Eg. in the tsserver log there were some exceptions that could result in projects being in incorrect state |
|
Well, at least in the case of #49961, if the error in the log before this one was in fact #49310, then I at least have a fix for that already. But, I think in the context of this code, it feels like we should at least either make the watcher potentially undefined and conditionally close it, or make it always required and remove the |
| watchMissingFileSystemEntry() : | ||
| watchPresentFileSystemEntry(); | ||
| return { | ||
| close: () => { | ||
| // Close the watcher (either existing file system entry watcher or missing file system entry watcher) | ||
| watcher.close(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But, my suggestion of
make it always required and remove the
undefined!and allowcloseto 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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
So, we should do what this PR does then, and ignore a second close?
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
Fixes #49961