Reduce amount of (un-needed) changed compile_commands.json events

[yiftahw]

closes #12889
Continuing on the discussion in #12911

• only monitor the compile_commands.json file pointed by the current configuration.
  • a timestamp is saved for each compile_commands.json path to keep track when we last notified on it's change.
  • on configuration selection change, we stat the file and compare to the saved timestamp to check if it changed while we were using a different configuration index.
• avoid checking the compile_commands.json file (both by fs.watcher and periodically) if a Configuration Provider is set, until we are notified that some file cannot be provided by that provider.
  • for example, some source file is not part of a CMakeLists.txt and we use the CMake Tools as the provider.
• periodic checking is done only if the compile_commands.json file watcher is inactive.
  • the configured path to compile_commands.json doesn't exist.
  • exceeded allowed amount of watchers on linux.
• for having an expected behavior on linux systems, the file watcher is closed on 'rename' events.
• successful manual checking retries to set up the file watcher.

Fixes that are byproducts of this PR:

1. checkCompileCommands() leaks open file watchers if the compileCommands file is deleted.

const compileCommandsFile: string | undefined = this.resolvePath(compileCommands);
fs.stat(compileCommandsFile, (err, stats) => {
    if (err) {
        if (err.code === "ENOENT" && this.compileCommandsFile) {
            this.compileCommandsFileWatchers = []; // reset file watchers <- leak here
            this.onCompileCommandsChanged(compileCommandsFile);
            this.compileCommandsFile = null; // File deleted
        }

2. due to the nature of the fallback function checkCompileCommands() only monitoring the current configuration's compile_commands.json with a single timestamp, we will not detect changes to a compile_commands.json on the following scenario:
   • we have more than 1 configuration, each one with a different path to a compile_commands.json.
   • the file watcher for the path in configuration y throwed an error. (for example if hit the inotify limit on Linux).
   • the file pointed by configuration y has changed while we were using configuration x.

{
    "configurations": [
        {
            "name": "x",
            "compileCommands": "${workspaceFolder}/compile_commands_x.json"
        },
        {
            "name": "y",
            "compileCommands": "${workspaceFolder}/compile_commands_y.json"
        }
    ]
}

[sean-mcmanus]

I think you can fix most of the linter errors by running the formatter.

Also, can you use sentence-style comments (starting with a capital letter and ending with a period)? We had one team member check in a bunch of util code that did not use sentence-style comments, but otherwise, I think most of our comments are sentence-styled.

It sounds like this PR doesn't need any cpptools-side changes?

[yiftahw]

@sean-mcmanus Hi, thanks for the review!

• lint errors fixed
• comments style fixed
• no need for changes in cpptools

[yiftahw]

@sean-mcmanus Added handling of the main scenario this merge request is removing:
Instead of monitoring all compile_commands.json files with fs.watch (if multiple configurations exist with each one pointing to a different file), we keep track of when each file was last checked. then, on configuration selection change, we check to see if the file the configuration points to has changed since the cached timestamp saved for that file.

[Colengms]

Is the primary issue the change is intended to address really an issue? The current implementation does not result in unnecessary loading or processing of commands_commands.json files. When an update notification is received on the native side and the current configuration is not set to use that specific compile_commands.json file, just a 'dirty' flag is set (associated with the configurations that use that file). While it may be useful to avoid sending unnecessary notifications to the native side, it doesn't seem like we're at risk of being flooded by these or doing significant unnecessary work.

That said, some feedback on code changes:

Updating compile_commands.json after a failed custom configuration request would be too late. Configurations are currently requested on the fly, in the process of starting up an IntelliSense translation unit. Native code would need to be updated to also wait for a (possible) update message related to a compile_commands.json file change (perhaps requiring an additional notification to indicate no change) before proceeding. That would seem to add another message (probably more often than the messages you're trying to avoid). Perhaps the response to the custom configuration request would indicate more information, to avoid additional messages, but I think there may be better overall approaches.

It's necessary to check isProviderRegistered to confirm that a configuration provider specified in the configuration has yet registered (as it may never). I suppose you'd then also need to react to the configuration provider becoming registered.

Could you perhaps extract the 'file watcher leak' fix into a separate PR?

[yiftahw]

@Colengms Thanks for the detailed explanation! It's kinda hard to dig deep on something when only a part of the code is open source.

Yes, I can start a PR on the leak. What about the 2nd scenario described? It's definitely a stretch to produce that edge case, but still technically can happen. Addressing it would mean keeping a timestamp per file instead of a single one (as used today). I'm referring to compileCommandsFileWatcherFallbackTime. Another solution is to poll all compile commands paths in checkCompileCommands() instead of only polling the one in the current configuration.

Insights are greatly appreciated.