Skip to content

Do not send first missing file event as well, Do not close typing installers watches just to recreate them #22520

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

Merged
merged 2 commits into from
Mar 16, 2018

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Mar 13, 2018

Handling as per #22496 (comment)

Fixes #22581

@sheetalkamat sheetalkamat changed the title Do not send first missing file event as well. Do not send first missing file event as well, Do not close typing installers watches just to recreate them Mar 15, 2018
@sheetalkamat
Copy link
Member Author

@amcasey can you please take a look. Thanks

@amcasey amcasey requested a review from RyanCavanaugh March 15, 2018 19:37
@amcasey
Copy link
Member

amcasey commented Mar 15, 2018

The missing file event part looks correct. I haven't had a chance to read the TI bit yet.

@RyanCavanaugh
Copy link
Member

What was the root cause of the bug? The code looks correct but the important behavioral difference between it and the old version isn't obvious.

@sheetalkamat
Copy link
Member Author

There were two causes
1: The delete event was sent on first invoke from the fileWatcher of missing files. (which we shouldnt)
2. Whenever the files are invalidated, we close all existing watches and create new watches which is expensive.. (it would be same list of files) which is what happens without the fix (since deleted is sent on first poll, invalidating the project, resulting in creating new install request which closes existing watch and sets new watcher which again sends file deleted event)

@sheetalkamat
Copy link
Member Author

@RyanCavanaugh does that answer your question, Is it ready to merge. I would like to get this in tonights nightly. Thanks

@sheetalkamat sheetalkamat merged commit d4788f5 into master Mar 16, 2018
@sheetalkamat sheetalkamat deleted the betterDelete branch March 16, 2018 18:17
DanielRosenwasser added a commit that referenced this pull request Mar 19, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants