Ensure errors for one path don't stop discovery of other paths

Author: robertbrignull

extensions/ql-vscode/src/common/discovery.ts

@@ -11,7 +11,10 @@ export abstract class Discovery extends DisposableObject {
   private restartWhenFinished = false;
   private currentDiscoveryPromise: Promise<void> | undefined;
 
-  constructor(private readonly name: string, private readonly logger: Logger) {
+  constructor(
+    protected readonly name: string,
+    private readonly logger: Logger,
+  ) {
     super();
   }
 

extensions/ql-vscode/src/common/vscode/file-path-discovery.ts

@@ -152,9 +152,19 @@ export abstract class FilePathDiscovery<T extends PathData> extends Discovery {
   protected async discover() {
     let pathsUpdated = false;
     for (const path of this.changedFilePaths) {
-      this.changedFilePaths.delete(path);
-      if (await this.handleChangedPath(path)) {
-        pathsUpdated = true;
+      try {
+        this.changedFilePaths.delete(path);
+        if (await this.handleChangedPath(path)) {
+          pathsUpdated = true;
+        }
+      } catch (e) {
+        // If we get an error while processing a path, just log it and continue.
+        // There aren't any network operations happening here or anything else
+        // that's likely to succeed on a retry, so don't bother adding it back
+        // to the changedFilePaths set.
+        void extLogger.log(
+          `${this.name} failed while processing path: ${path}`,
+        );
       }
     }
 

extensions/ql-vscode/test/vscode-tests/minimal-workspace/common/vscode/file-path-discovery.test.ts

@@ -11,6 +11,7 @@ import { basename, dirname, join } from "path";
 import { mkdirSync, readFileSync, rmSync, writeFileSync } from "fs";
 import * as tmp from "tmp";
 import { normalizePath } from "../../../../../src/pure/files";
+import { extLogger } from "../../../../../src/common/logging/vscode/loggers";
 
 interface TestData {
   path: string;
@@ -21,6 +22,9 @@ interface TestData {
  * A test FilePathDiscovery that operates on files with the ".test" extension.
  */
 class TestFilePathDiscovery extends FilePathDiscovery<TestData> {
+  public getDataForPathFunc: ((path: string) => Promise<TestData>) | undefined =
+    undefined;
+
   constructor() {
     super("TestFilePathDiscovery", "**/*.test");
   }
@@ -34,6 +38,9 @@ class TestFilePathDiscovery extends FilePathDiscovery<TestData> {
   }
 
   protected async getDataForPath(path: string): Promise<TestData> {
+    if (this.getDataForPathFunc !== undefined) {
+      return this.getDataForPathFunc(path);
+    }
     return {
       path,
       contents: readFileSync(path, "utf8"),
@@ -353,6 +360,39 @@ describe("FilePathDiscovery", () => {
     });
   });
 
+  describe("error handling", () => {
+    it("should handle errors and still process other files", async () => {
+      await discovery.initialRefresh();
+
+      discovery.getDataForPathFunc = async (path: string) => {
+        if (basename(path) === "123.test") {
+          throw new Error("error");
+        } else {
+          return { path, contents: readFileSync(path, "utf8") };
+        }
+      };
+      const logSpy = jest.spyOn(extLogger, "log");
+
+      makeTestFile(join(workspacePath, "123.test"));
+      makeTestFile(join(workspacePath, "456.test"));
+
+      onDidCreateFile.fire(Uri.file(join(workspacePath, "123.test")));
+      onDidCreateFile.fire(Uri.file(join(workspacePath, "456.test")));
+      await discovery.waitForCurrentRefresh();
+
+      expect(new Set(discovery.getPathData())).toEqual(
+        new Set([{ path: join(workspacePath, "456.test"), contents: "456" }]),
+      );
+
+      expect(logSpy).toHaveBeenCalledWith(
+        `TestFilePathDiscovery failed while processing path: ${join(
+          workspacePath,
+          "123.test",
+        )}`,
+      );
+    });
+  });
+
   describe("workspaceFoldersChanged", () => {
     it("initialRefresh establishes watchers", async () => {
       await discovery.initialRefresh();