diff --git a/denops/dpp/loader.ts b/denops/dpp/loader.ts index a386488..886ed89 100644 --- a/denops/dpp/loader.ts +++ b/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>; protocol: Record>; @@ -70,25 +64,80 @@ export class Loader { } async registerPath(type: DppExtType, path: string): Promise { - 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 { + 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( diff --git a/denops/dpp/tests/loader_concurrent_test.ts b/denops/dpp/tests/loader_concurrent_test.ts new file mode 100644 index 0000000..f9aaa45 --- /dev/null +++ b/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); +}); diff --git a/denops/dpp/tests/testdata/ext_alpha.ts b/denops/dpp/tests/testdata/ext_alpha.ts new file mode 100644 index 0000000..0019361 --- /dev/null +++ b/denops/dpp/tests/testdata/ext_alpha.ts @@ -0,0 +1,4 @@ +export class Ext { + name = ""; + path = ""; +} diff --git a/denops/dpp/tests/testdata/ext_beta.ts b/denops/dpp/tests/testdata/ext_beta.ts new file mode 100644 index 0000000..0019361 --- /dev/null +++ b/denops/dpp/tests/testdata/ext_beta.ts @@ -0,0 +1,4 @@ +export class Ext { + name = ""; + path = ""; +} diff --git a/denops/dpp/tests/testdata/ext_gamma.ts b/denops/dpp/tests/testdata/ext_gamma.ts new file mode 100644 index 0000000..0019361 --- /dev/null +++ b/denops/dpp/tests/testdata/ext_gamma.ts @@ -0,0 +1,4 @@ +export class Ext { + name = ""; + path = ""; +}