mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-10-27 11:35:23 -07:00
fix(plugins): unload late-loading plugins before quit teardown
- unloadAll waits for an in-progress load so plugins still activating get deactivated too - One 2 s budget covers the load wait and all deactivates - Quit cleanup awaits plugin unload before any other teardown - Force-quit timer now allows the plugin budget plus 2 s
This commit is contained in:
6 files changed
+92
-24
No files matched your search
@@ -67,7 +67,7 @@ A plugin folder needs a `plugin.json`:
|
||||
{ "name": "My Plugin", "version": "1.0.0", "main": "main.js" }
|
||||
```
|
||||
|
||||
`main` points to a CommonJS module (`.js`) or an ES module (`.mjs`) that exports `activate(ctx)`. It may return a cleanup function, or the module can export `deactivate()`. On quit, SubMiner waits up to 3 seconds for it, including any promise it returns. Everything a plugin registers through `ctx` (listeners, timers, actions, panels, running programs) is cleaned up automatically when SubMiner quits.
|
||||
`main` points to a CommonJS module (`.js`) or an ES module (`.mjs`) that exports `activate(ctx)`. It may return a cleanup function, or the module can export `deactivate()`. On quit, SubMiner waits up to 2 seconds for it, including any promise it returns. Everything a plugin registers through `ctx` (listeners, timers, actions, panels, running programs) is cleaned up automatically when SubMiner quits.
|
||||
|
||||
```js
|
||||
function activate(ctx) {
|
||||
|
||||
@@ -279,6 +279,30 @@ test('events follow a replacement mpv client', async () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('unload waits for an in-progress load so late plugins are unloaded too', async () => {
|
||||
const h = createHarness({ late: enabled() });
|
||||
const client = createFakeMpvClient();
|
||||
try {
|
||||
writePlugin(h.root, 'late', {
|
||||
'plugin.json': JSON.stringify({ main: 'main.cjs' }),
|
||||
'main.cjs': `exports.activate = (ctx) => {
|
||||
ctx.mpv.on('pause-change', () => ctx.notify('still listening'));
|
||||
return () => ctx.notify('deactivated');
|
||||
};`,
|
||||
});
|
||||
h.host.attachMpvClient(client);
|
||||
|
||||
// The plugin is still reading its manifest when unload starts.
|
||||
void h.host.loadAll();
|
||||
await h.host.unloadAll();
|
||||
client.emit('pause-change', { paused: true });
|
||||
|
||||
assert.deepEqual(h.notifications, ['deactivated']);
|
||||
} finally {
|
||||
h.cleanup();
|
||||
}
|
||||
});
|
||||
|
||||
test('runAction waits for a load that is still in progress', async () => {
|
||||
const h = createHarness({ slow: enabled() });
|
||||
try {
|
||||
|
||||
@@ -25,8 +25,11 @@ 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;
|
||||
/**
|
||||
* Quit waits for unloadAll, so a load or deactivate that never settles must not hold the app open.
|
||||
* The forced-exit fallback in main adds its own budget on top of this.
|
||||
*/
|
||||
export const PLUGIN_DEACTIVATE_TIMEOUT_MS = 2_000;
|
||||
|
||||
interface MpvClientEvents {
|
||||
on: (event: PluginMpvEventName, listener: (payload: unknown) => void) => void;
|
||||
@@ -54,7 +57,7 @@ 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. */
|
||||
/** How long unload waits, in total, for loading and deactivate before disposing anyway. */
|
||||
deactivateTimeoutMs?: number;
|
||||
}
|
||||
|
||||
@@ -66,6 +69,21 @@ interface LoadedPlugin {
|
||||
lifetime: AbortController;
|
||||
}
|
||||
|
||||
/** Resolves when `promise` settles, or rejects once `timeoutMs` passes first. */
|
||||
async function settleWithin(promise: Promise<unknown>, timeoutMs: number): Promise<void> {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
await Promise.race([
|
||||
promise,
|
||||
new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(() => reject(new Error(`timed out after ${timeoutMs} ms`)), timeoutMs);
|
||||
}),
|
||||
]);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
}
|
||||
|
||||
function errorMessage(error: unknown): string {
|
||||
return error instanceof Error ? error.message : String(error);
|
||||
}
|
||||
@@ -352,27 +370,31 @@ export function createPluginHost<TUnstable>(deps: PluginHostDeps<TUnstable>) {
|
||||
},
|
||||
|
||||
async unloadAll(): Promise<void> {
|
||||
// One budget covers the load wait and every deactivate, so quit is bounded by it.
|
||||
const timeoutMs = deps.deactivateTimeoutMs ?? PLUGIN_DEACTIVATE_TIMEOUT_MS;
|
||||
const deadline = Date.now() + timeoutMs;
|
||||
// Plugins still reading their files are not in `plugins` yet; let the load finish first.
|
||||
if (loadPromise) {
|
||||
try {
|
||||
await settleWithin(loadPromise, timeoutMs);
|
||||
} catch (error) {
|
||||
deps
|
||||
.createLogger('plugins')
|
||||
.warn(`Unloading before load finished: ${errorMessage(error)}`);
|
||||
}
|
||||
}
|
||||
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 Promise.race([
|
||||
await settleWithin(
|
||||
Promise.resolve().then(() => plugin.deactivate?.()),
|
||||
new Promise<never>((_, reject) => {
|
||||
timer = setTimeout(
|
||||
() => reject(new Error(`timed out after ${timeoutMs} ms`)),
|
||||
timeoutMs,
|
||||
);
|
||||
}),
|
||||
]);
|
||||
Math.max(0, deadline - Date.now()),
|
||||
);
|
||||
} catch (error) {
|
||||
deps
|
||||
.createLogger(`plugin:${plugin.id}`)
|
||||
.error(`deactivate failed: ${errorMessage(error)}`);
|
||||
} finally {
|
||||
clearTimeout(timer);
|
||||
}
|
||||
disposePlugin(plugin);
|
||||
}),
|
||||
|
||||
+6
-2
@@ -512,7 +512,10 @@ import { openSubsyncManualModal as openSubsyncManualModalRuntime } from './main/
|
||||
import { openSessionHelpModal as openSessionHelpModalRuntime } from './main/runtime/session-help-open';
|
||||
import { openChangelogModal as openChangelogModalRuntime } from './main/runtime/changelog-open';
|
||||
import { createChangelogRuntime } from './main/runtime/changelog/changelog-runtime';
|
||||
import { createPluginHost } from './core/services/plugins/plugin-host';
|
||||
import {
|
||||
createPluginHost,
|
||||
PLUGIN_DEACTIVATE_TIMEOUT_MS,
|
||||
} from './core/services/plugins/plugin-host';
|
||||
import { buildPluginSubtitleContext } from './core/services/plugins/subtitle-context';
|
||||
import type { PluginSubtitleLine } from './core/services/plugins/types';
|
||||
import {
|
||||
@@ -1054,6 +1057,7 @@ function requestAppQuit(): void {
|
||||
logger.warn('Failed to stop stats server while quitting.', error);
|
||||
});
|
||||
if (!forceQuitTimer) {
|
||||
// Quit cleanup unloads plugins first, so allow their budget plus 2 s for the rest.
|
||||
forceQuitTimer = setTimeout(() => {
|
||||
logger.warn('App quit timed out; forcing process exit.');
|
||||
void createForceQuitHandler({
|
||||
@@ -1061,7 +1065,7 @@ function requestAppQuit(): void {
|
||||
logError: (error) => logger.error('Failed to finalize stats before forced exit.', error),
|
||||
exit: () => app.exit(0),
|
||||
})();
|
||||
}, 2000);
|
||||
}, PLUGIN_DEACTIVATE_TIMEOUT_MS + 2000);
|
||||
}
|
||||
app.quit();
|
||||
}
|
||||
|
||||
@@ -100,7 +100,7 @@ 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 () => {
|
||||
test('on will quit cleanup handler finishes plugin unload before any other teardown', async () => {
|
||||
const calls: string[] = [];
|
||||
let finishUnload!: () => void;
|
||||
const cleanup = createOnWillQuitCleanupHandler(
|
||||
@@ -117,12 +117,28 @@ test('on will quit cleanup handler starts plugin unload first and waits for it t
|
||||
settled = true;
|
||||
});
|
||||
await new Promise((resolve) => setImmediate(resolve));
|
||||
assert.equal(calls[0], 'unloadPlugins');
|
||||
assert.deepEqual(calls, ['unloadPlugins']);
|
||||
assert.equal(settled, false);
|
||||
|
||||
finishUnload();
|
||||
await pending;
|
||||
assert.equal(settled, true);
|
||||
assert.ok(calls.indexOf('destroyMpvSocket') > 0);
|
||||
});
|
||||
|
||||
test('on will quit cleanup handler keeps tearing down when plugin unload fails', async () => {
|
||||
const calls: string[] = [];
|
||||
const cleanup = createOnWillQuitCleanupHandler(
|
||||
makeCleanupDeps(calls, {
|
||||
unloadPlugins: async () => {
|
||||
throw new Error('unload failed');
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
await assert.rejects(cleanup(), /unload failed/);
|
||||
assert.ok(calls.includes('destroyMpvSocket'));
|
||||
assert.ok(calls.includes('stopDiscordPresenceService'));
|
||||
});
|
||||
|
||||
test('on will quit cleanup handler cleans subtitle caches when stopping remote session fails', async () => {
|
||||
|
||||
@@ -63,10 +63,13 @@ export function createOnWillQuitCleanupHandler(deps: {
|
||||
}) {
|
||||
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) => {
|
||||
// Plugins deactivate before any teardown so they can still reach mpv and the overlay;
|
||||
// the plugin host bounds how long that takes.
|
||||
try {
|
||||
await deps.unloadPlugins();
|
||||
} catch (error) {
|
||||
cleanupErrors.push(error);
|
||||
});
|
||||
}
|
||||
deps.destroyTray();
|
||||
deps.stopConfigHotReload();
|
||||
deps.restorePreviousSecondarySubVisibility();
|
||||
@@ -123,7 +126,6 @@ export function createOnWillQuitCleanupHandler(deps: {
|
||||
deps.cleanupRemoteMediaWindows();
|
||||
deps.stopDiscordPresenceService();
|
||||
await stopSyncAutoScheduler;
|
||||
await unloadPlugins;
|
||||
if (cleanupErrors.length > 0) {
|
||||
throw cleanupErrors[0];
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user