mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-10-27 11:35:23 -07:00
fix(plugins): await plugin unload on quit with deactivate timeout
- Move plugin unload into the will-quit cleanup handler so it starts first and quit waits for it - Give each plugin's deactivate 3 s (configurable via deactivateTimeoutMs) before disposing it anyway - Document the quit timeout in docs-site/plugins.md
This commit is contained in:
8 files changed
+84
-5
No files matched your search
@@ -42,7 +42,10 @@ function createFakeWindow() {
|
||||
return fake;
|
||||
}
|
||||
|
||||
function createHarness(config: Record<string, PluginConfigEntry>) {
|
||||
function createHarness(
|
||||
config: Record<string, PluginConfigEntry>,
|
||||
overrides: Partial<PluginHostDeps<{ marker: string }>> = {},
|
||||
) {
|
||||
const root = mkdtempSync(path.join(os.tmpdir(), 'subminer-plugins-'));
|
||||
const errors: string[] = [];
|
||||
const notifications: string[] = [];
|
||||
@@ -82,6 +85,7 @@ function createHarness(config: Record<string, PluginConfigEntry>) {
|
||||
return { window: window.asWindow(), created: true };
|
||||
},
|
||||
unstable: { marker: 'internals' },
|
||||
...overrides,
|
||||
};
|
||||
return {
|
||||
root: deps.pluginsDir,
|
||||
@@ -227,6 +231,30 @@ test('mpv listener errors are contained and unload detaches listeners and calls
|
||||
}
|
||||
});
|
||||
|
||||
test('unload gives up on a deactivate that never settles and still disposes the plugin', async () => {
|
||||
const h = createHarness({ stuck: enabled() }, { deactivateTimeoutMs: 20 });
|
||||
const client = createFakeMpvClient();
|
||||
try {
|
||||
writePlugin(h.root, 'stuck', {
|
||||
'plugin.json': JSON.stringify({ main: 'main.cjs' }),
|
||||
'main.cjs': `exports.activate = (ctx) => {
|
||||
ctx.mpv.on('pause-change', () => ctx.notify('still listening'));
|
||||
return () => new Promise(() => {});
|
||||
};`,
|
||||
});
|
||||
h.host.attachMpvClient(client);
|
||||
await h.host.loadAll();
|
||||
|
||||
await h.host.unloadAll();
|
||||
client.emit('pause-change', { paused: true });
|
||||
|
||||
assert.deepEqual(h.notifications, []);
|
||||
assert.deepEqual(h.errors, ['deactivate failed: timed out after 20 ms']);
|
||||
} finally {
|
||||
h.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
test('events follow a replacement mpv client', async () => {
|
||||
const h = createHarness({ watcher: enabled() });
|
||||
const first = createFakeMpvClient();
|
||||
|
||||
@@ -25,6 +25,8 @@ import {
|
||||
const MIN_TIMER_INTERVAL_MS = 100;
|
||||
const DEFAULT_WINDOW_WIDTH = 960;
|
||||
const DEFAULT_WINDOW_HEIGHT = 720;
|
||||
// Quit waits for unloadAll, so a deactivate that never settles must not hold the app open.
|
||||
const DEFAULT_DEACTIVATE_TIMEOUT_MS = 3_000;
|
||||
|
||||
interface MpvClientEvents {
|
||||
on: (event: PluginMpvEventName, listener: (payload: unknown) => void) => void;
|
||||
@@ -52,6 +54,8 @@ export interface PluginHostDeps<TUnstable> {
|
||||
/** Test seam; defaults to Node's `require`, which also loads synchronous ES modules. */
|
||||
loadModule?: (entryPath: string) => PluginModule;
|
||||
exec?: typeof execPluginCommand;
|
||||
/** How long unload waits for each plugin's deactivate before disposing it anyway. */
|
||||
deactivateTimeoutMs?: number;
|
||||
}
|
||||
|
||||
interface LoadedPlugin {
|
||||
@@ -349,14 +353,26 @@ export function createPluginHost<TUnstable>(deps: PluginHostDeps<TUnstable>) {
|
||||
|
||||
async unloadAll(): Promise<void> {
|
||||
loadPromise = null;
|
||||
const timeoutMs = deps.deactivateTimeoutMs ?? DEFAULT_DEACTIVATE_TIMEOUT_MS;
|
||||
await Promise.all(
|
||||
[...plugins.values()].map(async (plugin) => {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
await plugin.deactivate?.();
|
||||
await Promise.race([
|
||||
Promise.resolve().then(() => plugin.deactivate?.()),
|
||||
new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(
|
||||
() => reject(new Error(`timed out after ${timeoutMs} ms`)),
|
||||
timeoutMs,
|
||||
);
|
||||
}),
|
||||
]);
|
||||
} catch (error) {
|
||||
deps
|
||||
.createLogger(`plugin:${plugin.id}`)
|
||||
.error(`deactivate failed: ${errorMessage(error)}`);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
disposePlugin(plugin);
|
||||
}),
|
||||
|
||||
+1
-1
@@ -4403,6 +4403,7 @@ const {
|
||||
void appState.discordPresenceService?.stop();
|
||||
appState.discordPresenceService = null;
|
||||
},
|
||||
unloadPlugins: () => pluginHost.unloadAll(),
|
||||
},
|
||||
shouldRestoreWindowsOnActivateMainDeps: {
|
||||
isOverlayRuntimeInitialized: () => appState.overlayRuntimeInitialized,
|
||||
@@ -7067,4 +7068,3 @@ registerSubtitleGenerationIpc({
|
||||
openModal: () => openSubtitleGenerationModal(createOverlayHostedModalOpenDeps()),
|
||||
});
|
||||
app.on('before-quit', () => subtitleGenerationRuntime.cancel());
|
||||
app.on('before-quit', () => void pluginHost.unloadAll());
|
||||
@@ -55,6 +55,7 @@ function makeCleanupDeps(calls: string[], overrides: Partial<CleanupDeps> = {}):
|
||||
cleanupRemoteMediaWindows: record('cleanupRemoteMediaWindows'),
|
||||
cleanupJellyfinSubtitleCache: record('cleanupJellyfinSubtitleCache'),
|
||||
stopDiscordPresenceService: record('stopDiscordPresenceService'),
|
||||
unloadPlugins: record('unloadPlugins'),
|
||||
...overrides,
|
||||
};
|
||||
}
|
||||
@@ -99,6 +100,31 @@ test('on will quit cleanup handler flushes before teardown and awaits stats befo
|
||||
assert.ok(calls.indexOf('destroyImmersionTracker') < calls.indexOf('destroyAnkiIntegration'));
|
||||
});
|
||||
|
||||
test('on will quit cleanup handler starts plugin unload first and waits for it to settle', async () => {
|
||||
const calls: string[] = [];
|
||||
let finishUnload!: () => void;
|
||||
const cleanup = createOnWillQuitCleanupHandler(
|
||||
makeCleanupDeps(calls, {
|
||||
unloadPlugins: () => {
|
||||
calls.push('unloadPlugins');
|
||||
return new Promise<void>((resolve) => (finishUnload = resolve));
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
let settled = false;
|
||||
const pending = cleanup().then(() => {
|
||||
settled = true;
|
||||
});
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
assert.equal(calls[0], 'unloadPlugins');
|
||||
assert.equal(settled, false);
|
||||
|
||||
finishUnload();
|
||||
await pending;
|
||||
assert.equal(settled, true);
|
||||
});
|
||||
|
||||
test('on will quit cleanup handler cleans subtitle caches when stopping remote session fails', async () => {
|
||||
const calls: string[] = [];
|
||||
const cleanup = createOnWillQuitCleanupHandler(
|
||||
|
||||
@@ -59,8 +59,14 @@ export function createOnWillQuitCleanupHandler(deps: {
|
||||
cleanupRemoteMediaWindows: () => void;
|
||||
cleanupJellyfinSubtitleCache: () => void;
|
||||
stopDiscordPresenceService: () => void;
|
||||
unloadPlugins: () => void | Promise<void>;
|
||||
}) {
|
||||
return async (): Promise<void> => {
|
||||
const cleanupErrors: unknown[] = [];
|
||||
// Plugins deactivate first, while the app is still intact; the host bounds how long that takes.
|
||||
const unloadPlugins = (async () => deps.unloadPlugins())().catch((error: unknown) => {
|
||||
cleanupErrors.push(error);
|
||||
});
|
||||
deps.destroyTray();
|
||||
deps.stopConfigHotReload();
|
||||
deps.restorePreviousSecondarySubVisibility();
|
||||
@@ -68,7 +74,6 @@ export function createOnWillQuitCleanupHandler(deps: {
|
||||
deps.unregisterAllGlobalShortcuts();
|
||||
deps.stopSubtitleWebsocket();
|
||||
deps.stopTexthookerService();
|
||||
const cleanupErrors: unknown[] = [];
|
||||
const stopSyncAutoScheduler = Promise.resolve(deps.stopSyncAutoScheduler()).catch(
|
||||
(error: unknown) => {
|
||||
cleanupErrors.push(error);
|
||||
@@ -118,6 +123,7 @@ export function createOnWillQuitCleanupHandler(deps: {
|
||||
deps.cleanupRemoteMediaWindows();
|
||||
deps.stopDiscordPresenceService();
|
||||
await stopSyncAutoScheduler;
|
||||
await unloadPlugins;
|
||||
if (cleanupErrors.length > 0) {
|
||||
throw cleanupErrors[0];
|
||||
}
|
||||
|
||||
@@ -52,6 +52,7 @@ function buildCleanupDeps(overrides: Partial<BuilderDeps> = {}) {
|
||||
cleanupRemoteMediaWindows: noop,
|
||||
cleanupJellyfinSubtitleCache: noop,
|
||||
stopDiscordPresenceService: noop,
|
||||
unloadPlugins: noop,
|
||||
...overrides,
|
||||
})();
|
||||
}
|
||||
|
||||
@@ -65,6 +65,7 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: {
|
||||
cleanupRemoteMediaWindows: () => void;
|
||||
cleanupJellyfinSubtitleCache: () => void;
|
||||
stopDiscordPresenceService: () => void;
|
||||
unloadPlugins: () => void | Promise<void>;
|
||||
}) {
|
||||
return () => ({
|
||||
destroyTray: () => deps.destroyTray(),
|
||||
@@ -154,5 +155,6 @@ export function createBuildOnWillQuitCleanupDepsHandler(deps: {
|
||||
cleanupRemoteMediaWindows: () => deps.cleanupRemoteMediaWindows(),
|
||||
cleanupJellyfinSubtitleCache: () => deps.cleanupJellyfinSubtitleCache(),
|
||||
stopDiscordPresenceService: () => deps.stopDiscordPresenceService(),
|
||||
unloadPlugins: () => deps.unloadPlugins(),
|
||||
});
|
||||
}
|
||||
Reference in new issue
Block a user