refactor(loader): parallelize I/O in registerPath; add registerPaths

denops/dpp/loader.ts

@@ -13,12 +13,6 @@ import { join } from "@std/path/join";
 import { parse } from "@std/path/parse";
 import { Lock } from "@core/asyncutil/lock";
 
-type Mod = {
-  // deno-lint-ignore no-explicit-any
-  mod: any;
-  path: string;
-};
-
 type Ext = {
   ext: Record<string, BaseExt<BaseParams>>;
   protocol: Record<string, BaseProtocol<BaseParams>>;
@@ -70,25 +64,80 @@ export class Loader {
   }
 
   async registerPath(type: DppExtType, path: string): Promise<void> {
-    await this.#registerLock.lock(async () => {
-      try {
-        await this.#register(type, path);
-      } catch (e) {
-        if (isDenoCacheIssueError(e)) {
-          console.warn("*".repeat(80));
-          console.warn(`Deno module cache issue is detected.`);
-          console.warn(
-            `Execute '!deno cache --reload "${path}"' and restart Vim/Neovim.`,
-          );
-          console.warn("*".repeat(80));
-        }
+    // Fast-path: skip I/O if already registered.
+    if (path in this.#checkPaths) {
+      return;
+    }
 
-        console.error(`Failed to load file '${path}': ${e}`);
-        throw e;
+    const name = parse(path).name;
+
+    // Perform I/O outside the lock so concurrent calls run in parallel.
+    // NOTE: We intentionally use Deno.stat instead of safeStat here. We expect
+    // errors to be thrown when paths don't exist or are inaccessible.
+    // deno-lint-ignore no-explicit-any
+    let importedMod: any;
+    try {
+      const fileInfo = await Deno.stat(path);
+      const entryPoint = fileInfo.isDirectory
+        ? join(path, EXT_ENTRY_POINT_FILE)
+        : path;
+      importedMod = await importPlugin(entryPoint);
+    } catch (e) {
+      if (isDenoCacheIssueError(e)) {
+        console.warn("*".repeat(80));
+        console.warn(`Deno module cache issue is detected.`);
+        console.warn(
+          `Execute '!deno cache --reload "${path}"' and restart Vim/Neovim.`,
+        );
+        console.warn("*".repeat(80));
       }
+
+      console.error(`Failed to load file '${path}': ${e}`);
+      throw e;
+    }
+
+    // Update shared state under lock; re-check to avoid duplicate registration
+    // by concurrent calls that passed the fast-path check simultaneously.
+    await this.#registerLock.lock(() => {
+      if (path in this.#checkPaths) {
+        return;
+      }
+
+      const typeExt = this.#exts[type];
+      switch (type) {
+        case "ext": {
+          const ext = new importedMod.Ext();
+          ext.name = name;
+          ext.path = path;
+          typeExt[name] = ext;
+          break;
+        }
+        case "protocol": {
+          const ext = new importedMod.Protocol();
+          ext.name = name;
+          ext.path = path;
+          typeExt[name] = ext;
+          break;
+        }
+      }
+
+      this.#checkPaths[path] = true;
     });
   }
 
+  async registerPaths(type: DppExtType, paths: string[]): Promise<void> {
+    const results = await Promise.allSettled(
+      paths.map((path) => this.registerPath(type, path)),
+    );
+    for (const result of results) {
+      if (result.status === "rejected") {
+        console.error(
+          `registerPaths: failed to register a path: ${result.reason}`,
+        );
+      }
+    }
+  }
+
   registerExtension(
     type: "ext",
     name: string,
@@ -130,56 +179,6 @@ export class Loader {
 
     return this.#exts.protocol[name];
   }
-
-  async #register(type: DppExtType, path: string) {
-    if (path in this.#checkPaths) {
-      return;
-    }
-
-    const name = parse(path).name;
-    const mod: Mod = {
-      mod: undefined,
-      path,
-    };
-
-    // NOTE: We intentionally use Deno.stat instead of safeStat here. We expect
-    // errors to be thrown when paths don't exist or are inaccessible.
-    const fileInfo = await Deno.stat(path);
-
-    if (fileInfo.isDirectory) {
-      // Load structured extension module
-      const entryPoint = join(path, EXT_ENTRY_POINT_FILE);
-      mod.mod = await importPlugin(entryPoint);
-    } else {
-      // Load single-file extension module
-      mod.mod = await importPlugin(path);
-    }
-
-    const typeExt = this.#exts[type];
-    let add;
-    switch (type) {
-      case "ext":
-        add = (name: string) => {
-          const ext = new mod.mod.Ext();
-          ext.name = name;
-          ext.path = path;
-          typeExt[name] = ext;
-        };
-        break;
-      case "protocol":
-        add = (name: string) => {
-          const ext = new mod.mod.Protocol();
-          ext.name = name;
-          ext.path = path;
-          typeExt[name] = ext;
-        };
-        break;
-    }
-
-    add(name);
-
-    this.#checkPaths[path] = true;
-  }
 }
 
 async function createPathCache(

denops/dpp/tests/loader_concurrent_test.ts

@@ -0,0 +1,149 @@
+import { assertEquals } from "@std/assert/equals";
+import { assertRejects } from "@std/assert/rejects";
+import { fromFileUrl } from "@std/path/from-file-url";
+import { toFileUrl } from "@std/path/to-file-url";
+import { join } from "@std/path/join";
+import { Loader } from "../loader.ts";
+
+const TEST_DATA_DIR = join(
+  fromFileUrl(new URL(import.meta.url)),
+  "..",
+  "testdata",
+);
+
+function fixturePath(name: string): string {
+  return join(TEST_DATA_DIR, `${name}.ts`);
+}
+
+Deno.test("registerPath: registers a single ext path without error", async () => {
+  const loader = new Loader();
+  const path = fixturePath("ext_alpha");
+  await loader.registerPath("ext", path);
+});
+
+Deno.test(
+  "registerPath: concurrent registration of the same path is idempotent",
+  async () => {
+    const loader = new Loader();
+    const path = fixturePath("ext_alpha");
+
+    // Fire 8 concurrent registrations for the same path.
+    const results = await Promise.allSettled(
+      Array.from({ length: 8 }, () => loader.registerPath("ext", path)),
+    );
+
+    // All should resolve (no rejections despite concurrent calls).
+    const rejected = results.filter((r) => r.status === "rejected");
+    assertEquals(rejected.length, 0, "No registration should fail");
+  },
+);
+
+Deno.test(
+  "registerPath: concurrent registration of different paths succeeds",
+  async () => {
+    const loader = new Loader();
+    const paths = [
+      fixturePath("ext_alpha"),
+      fixturePath("ext_beta"),
+      fixturePath("ext_gamma"),
+    ];
+
+    const results = await Promise.allSettled(
+      paths.map((p) => loader.registerPath("ext", p)),
+    );
+
+    const rejected = results.filter((r) => r.status === "rejected");
+    assertEquals(
+      rejected.length,
+      0,
+      "All different-path registrations should succeed",
+    );
+  },
+);
+
+Deno.test(
+  "registerPath: registering a non-existent path throws",
+  async () => {
+    const loader = new Loader();
+    const badPath = join(TEST_DATA_DIR, "nonexistent.ts");
+
+    await assertRejects(
+      () => loader.registerPath("ext", badPath),
+      Error,
+    );
+  },
+);
+
+Deno.test("registerPaths: registers multiple paths concurrently", async () => {
+  const loader = new Loader();
+  const paths = [
+    fixturePath("ext_alpha"),
+    fixturePath("ext_beta"),
+    fixturePath("ext_gamma"),
+  ];
+
+  // registerPaths should not throw even on success.
+  await loader.registerPaths("ext", paths);
+});
+
+Deno.test(
+  "registerPaths: does not throw when some paths are invalid",
+  async () => {
+    const loader = new Loader();
+    const paths = [
+      fixturePath("ext_alpha"),
+      join(TEST_DATA_DIR, "nonexistent.ts"),
+      fixturePath("ext_beta"),
+    ];
+
+    // registerPaths absorbs errors internally and must not throw.
+    await loader.registerPaths("ext", paths);
+  },
+);
+
+Deno.test(
+  "registerPaths: concurrent calls for the same paths are idempotent",
+  async () => {
+    const loader = new Loader();
+    const paths = [fixturePath("ext_alpha"), fixturePath("ext_beta")];
+
+    // Two concurrent registerPaths calls with overlapping paths.
+    const [r1, r2] = await Promise.allSettled([
+      loader.registerPaths("ext", paths),
+      loader.registerPaths("ext", paths),
+    ]);
+
+    assertEquals(r1.status, "fulfilled");
+    assertEquals(r2.status, "fulfilled");
+  },
+);
+
+Deno.test(
+  "registerPath: re-registration of the same path after initial success is a no-op",
+  async () => {
+    const loader = new Loader();
+    const path = fixturePath("ext_alpha");
+
+    await loader.registerPath("ext", path);
+    // Second call must succeed without error (fast-path check).
+    await loader.registerPath("ext", path);
+  },
+);
+
+// Verify that the module URL used in import.meta.url resolves to a real file.
+Deno.test("testdata fixtures are accessible", async () => {
+  for (const name of ["ext_alpha", "ext_beta", "ext_gamma"]) {
+    const p = fixturePath(name);
+    const stat = await Deno.stat(p);
+    assertEquals(stat.isFile, true, `${name}.ts should be a file`);
+  }
+});
+
+// Smoke-test: ensure toFileUrl/fromFileUrl round-trips work correctly for
+// fixture paths (importPlugin relies on toFileUrl internally).
+Deno.test("fixture paths survive toFileUrl/fromFileUrl round-trip", () => {
+  const path = fixturePath("ext_alpha");
+  const url = toFileUrl(path).href;
+  const back = fromFileUrl(url);
+  assertEquals(back, path);
+});

denops/dpp/tests/testdata/ext_alpha.ts

@@ -0,0 +1,4 @@
+export class Ext {
+  name = "";
+  path = "";
+}

denops/dpp/tests/testdata/ext_beta.ts

@@ -0,0 +1,4 @@
+export class Ext {
+  name = "";
+  path = "";
+}

denops/dpp/tests/testdata/ext_gamma.ts

@@ -0,0 +1,4 @@
+export class Ext {
+  name = "";
+  path = "";
+}