Repository navigation
fix: resolve all waiters for plugin loads - #2955
mikamikasuki wants to merge 1 commit into
Conversation
 |
| ); | ||
| } | ||
| this.#pluginWatchers = {}; | ||
| this.#pluginWatchers.rejectAll( |
There was a problem hiding this comment.
This callback also fires at the end of the theme-only pass (loadPlugins(true) in main.js:766), which runs before the main loadPlugins() pass. Any waiter registered by then (e.g. by a theme plugin waiting on a regular plugin) gets rejected with "failed to load" even though that plugin hasn't been attempted yet.
Could we only reject when !loadOnlyTheme, or pass that flag through to the callback?
| this.#pluginWatchers.resolve(pluginId); | ||
| } | ||
|
|
||
| [onPluginsLoadCompleteCallback]() { |
There was a problem hiding this comment.
Timed-out plugins: loadPluginWithTimeout returns false after 15s but keeps loading in the background. loadPlugins then completes and rejects its waiters here. When the plugin finishes later, markPluginLoaded → resolve(pluginId) finds nothing, so dependents never initialize even though the plugin is running.
Maybe skip rejecting plugins that timed out but haven't settled, or defer their rejection until PLUGIN_DISABLE_TIMEOUT.
| ); | ||
| } | ||
|
|
||
| waitForPlugin(pluginId) { |
There was a problem hiding this comment.
If the initial load has already completed (isInitialPluginLoadComplete()), a call for a plugin that's broken/disabled/not installed registers a waiter that will never settle — no further onPluginsLoadCompleteCallback runs, so the caller hangs silently.
Since this function is being rewritten anyway, it'd be good to reject immediately in that case.
Related: with the new Set, repeated calls for a never-loaded ID now accumulate entries for the rest of the session (previously it was overwritten, so bounded at one per ID). Settling late callers immediately fixes both.
| reject, | ||
| }; | ||
| }); | ||
| if (LOADED_PLUGINS.has(pluginId)) return Promise.resolve(true); |
There was a problem hiding this comment.
Nit: the fast path resolves with true, but the waiter path resolves with undefined (waiter.resolve() in PluginWaiters). Code like if (await acode.waitForPlugin("dep")) init(); behaves differently depending on load order. Suggest waiter.resolve(true) and updating the test accordingly.
| waiters = new Set(); | ||
| this.#waiters.set(pluginId, waiters); | ||
| } | ||
| waiters.add({ resolve, reject }); |
There was a problem hiding this comment.
Suggestion: rather than a Set of separate { resolve, reject } objects, store one deferred per plugin ID and hand every caller the same promise:
waitFor(pluginId) {
let deferred = this.#waiters.get(pluginId);
if (!deferred) {
deferred = Promise.withResolvers();
this.#waiters.set(pluginId, deferred);
}
return deferred.promise;
}resolve/rejectAll then become single calls with no inner loops. (Check Promise.withResolvers support on the minimum WebView; a small manual deferred works too.)
| rejectAll(getError) { | ||
| for (const [pluginId, waiters] of this.#waiters) { | ||
| this.#waiters.delete(pluginId); | ||
| for (const waiter of waiters) waiter.reject(getError(pluginId)); |
There was a problem hiding this comment.
getError(pluginId) is called per waiter inside the loop, after the entry has already been deleted. If it ever throws, the remaining waiters for this plugin (and all later plugins) are stranded, and the exception propagates into loadPlugins. Building the error once per plugin before the inner loop avoids that and avoids redundant allocations.
| @@ -0,0 +1,31 @@ | |||
| import { expect, it } from "vitest"; | |||
There was a problem hiding this comment.
These tests cover PluginWaiters in isolation, but none of the interesting failure modes are here — they're in how acode.waitForPlugin is wired to loadPlugins (theme-only pass rejection, timed-out plugins that load later, calls after initial load). Would be worth adding at least one test exercising waitForPlugin + the load callbacks.
| const pending = Promise.allSettled([first, second, third]); | ||
|
|
||
| waiters.rejectAll((pluginId) => new Error(`Plugin '${pluginId}' failed to load.`)); | ||
|
|
There was a problem hiding this comment.
Nit: this file isn't Biome-formatted (several lines exceed the 80-col width). It slips through because biome.json files.includes only covers src/** and utils/**, so the biome check mentioned in the PR description doesn't actually check it. Running biome format --write on it explicitly would keep it consistent.
2 participants
Fixes #2936
waitForPlugin()stored one resolver per plugin ID. When multiple callers waited for the same plugin, later calls replaced earlier resolvers and left those promises pending.This change keeps all pending callers for each plugin ID, resolves them when that plugin loads, and rejects them all with the existing plugin-specific error when initial plugin loading completes. The already-loaded fast path remains unchanged.
Regression coverage verifies that multiple callers resolve together and that all waiters receive the correct error when loading fails.
Validation:
vitest run tests/unit/pluginWaiters.test.jsbiome check src/lib/acode.js src/lib/pluginWaiters.js tests/unit/pluginWaiters.test.jsgit diff --check