feedback7 R22-HUD: a renderer that loses the 12 s race no longer mounts its canvas late — the attempt is aborted, a view that still comes back is destroyed, the host is no longer wiped (PR #302 by @siruimei07); golden: unchanged
Applied unchanged after review (Grok static review: MERGE; the diff read in full: no network, no new dependency, no
shipped eval). The leak was real on feedback7: mountFieldView raced createFieldView against a timeout and, on failure,
removed every child of the host while the factory kept running — Pixi finished later and appended its canvas over the
simplified view (taking its clicks); the host wipe could also delete a newer view mounted by the same hook.
- ui/fieldHost.js: mountFieldView(host, { signal }) owns an AbortController; the timeout, a failed import or an invalid
view aborts it, a view a factory returns after that is destroyed, an outer abort (useFieldView's cleanup) rejects
without a fallback; nothing clears the host's children any more.
- render/app.js createFieldView takes the signal: the Pixi load / manifest / font waits end on abort, the synchronous
build registers its disposables (setupCleanup) so a throw releases the app, tiles, fx, impostors, listeners and
observers it made, the board-art and 3D-board waits moved after the view exists (aborting them is view.destroy(),
idempotent), destroy() also removes the canvas. render/app/host.js withTimeout(p, ms, signal).
- Tests: test/ui/fieldHost-lifecycle.test.js (5, node vm, fake timers: all fail on the parent) and the opt-in real-browser
test/render/initialization.browser.test.js (6 scenarios in headless Chrome, 5 fail on the parent: a late Pixi load
covering the fallback — with a factory that honours the signal and one that ignores it —, art timeout, a setup
exception, an unmount racing a newer mount); test/ui/playtest2.test.js's source pin takes the new indentation.
fieldview / browser suites in Chrome unchanged (the two 4× throttled fps tests flake when two Chrome files run
at once, pass alone).
- The createFieldView body stays unindented inside its new try (the PR's choice, to keep the diff reviewable).
- Docs: design/client.md §9. Golden: unchanged (client lifecycle only).
This commit is contained in:
1 parent
8ae891f1f3
commit
030d0a2938
7 files changed
+654
-39
No files matched your search
@@ -0,0 +1,158 @@
|
||||
// Field-host ownership under cancellation and timeout. Browser tests cover the actual renderer; this fixture runs
|
||||
// the production orchestration with controlled imports and timers, without a DOM or GPU dependency.
|
||||
|
||||
import { test } from 'node:test';
|
||||
import assert from 'node:assert/strict';
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { getEventListeners } from 'node:events';
|
||||
import vm from 'node:vm';
|
||||
|
||||
const sourcePath = new URL('../../public/js/ui/fieldHost.js', import.meta.url);
|
||||
const source = readFileSync(sourcePath, 'utf8')
|
||||
.replace(/^import .*;$/gm, '').replace(/^export /gm, '')
|
||||
.replace("import('../assets.js')", 'importAssets()')
|
||||
.replace("import('../render/app.js')", 'importApp()');
|
||||
const METHODS = ['setStage', 'setCamera', 'setPrep', 'enterBattle', 'pushSnapshot', 'pushEvents', 'highlightTiles', 'on', 'resize'];
|
||||
const flush = async () => { for (let i = 0; i < 16; i++) await Promise.resolve(); };
|
||||
|
||||
function fixture() {
|
||||
const timers = new Map(), requests = [], fallbacks = [], warnings = [];
|
||||
let timerId = 0;
|
||||
const unrelated = { name: 'unrelated' };
|
||||
const host = {
|
||||
children: [unrelated],
|
||||
get firstChild() { return this.children[0] || null; },
|
||||
appendChild(node) { this.children.push(node); },
|
||||
removeChild(node) { const i = this.children.indexOf(node); if (i >= 0) this.children.splice(i, 1); },
|
||||
};
|
||||
function view(name, valid = true) {
|
||||
const node = { name };
|
||||
host.appendChild(node);
|
||||
return {
|
||||
...(valid ? Object.fromEntries(METHODS.map((k) => [k, () => {}])) : {}),
|
||||
node, destroyCalls: 0,
|
||||
destroy() { this.destroyCalls++; host.removeChild(node); },
|
||||
};
|
||||
}
|
||||
const context = vm.createContext({
|
||||
AbortController, DOMException, URLSearchParams, Promise,
|
||||
console: { warn: (...args) => warnings.push(args) },
|
||||
audio: {}, data: { get() { return null; } }, settingsStore: { get() { return {}; } },
|
||||
setTimeout(fn) { timers.set(++timerId, fn); return timerId; },
|
||||
clearTimeout(id) { timers.delete(id); },
|
||||
importAssets: async () => null,
|
||||
importApp: async () => ({
|
||||
createFieldView(_host, opts) {
|
||||
// Deliberately ignore cancellation: the host must still dispose a factory's late return value.
|
||||
return new Promise((resolve, reject) => requests.push({ resolve, reject, signal: opts.signal }));
|
||||
},
|
||||
}),
|
||||
createFallbackView() {
|
||||
const fallback = view('fallback');
|
||||
fallbacks.push({ view: fallback, rendererAborted: requests.at(-1)?.signal?.aborted });
|
||||
return fallback;
|
||||
},
|
||||
});
|
||||
vm.runInContext(source + '\nglobalThis.mount = mountFieldView;', context, { filename: sourcePath.pathname });
|
||||
return {
|
||||
host, unrelated, timers, requests, fallbacks, warnings, view,
|
||||
mount: (signal) => context.mount(host, { signal }),
|
||||
timeout() { const fn = [...timers.values()].at(-1); assert.ok(fn, 'initialization has a deadline'); fn(); },
|
||||
};
|
||||
}
|
||||
|
||||
function settled(promise) {
|
||||
const result = { state: 'pending', value: null };
|
||||
promise.then((value) => { result.state = 'fulfilled'; result.value = value; }, (error) => { result.state = 'rejected'; result.value = error; });
|
||||
return result;
|
||||
}
|
||||
|
||||
test('successful initialization releases deadlines and cancellation listeners', async () => {
|
||||
const f = fixture(), controller = new AbortController();
|
||||
const pending = f.mount(controller.signal);
|
||||
await flush();
|
||||
const raw = f.view('engine');
|
||||
f.requests[0].resolve(raw);
|
||||
const mounted = await pending;
|
||||
assert.equal(mounted.kind, 'engine');
|
||||
assert.equal(f.timers.size, 0);
|
||||
assert.equal(getEventListeners(controller.signal, 'abort').length, 0);
|
||||
assert.equal(getEventListeners(f.requests[0].signal, 'abort').length, 0);
|
||||
mounted.destroy();
|
||||
assert.equal(raw.destroyCalls, 1);
|
||||
});
|
||||
|
||||
test('cancelling a pending mount rejects without fallback and disposes a late result', async () => {
|
||||
const f = fixture(), controller = new AbortController();
|
||||
const result = settled(f.mount(controller.signal));
|
||||
await flush();
|
||||
controller.abort();
|
||||
await flush();
|
||||
assert.equal(result.state, 'rejected');
|
||||
assert.equal(result.value.name, 'AbortError');
|
||||
assert.equal(f.fallbacks.length, 0);
|
||||
assert.equal(f.warnings.length, 0);
|
||||
assert.equal(f.timers.size, 0);
|
||||
assert.equal(getEventListeners(controller.signal, 'abort').length, 0);
|
||||
const late = f.view('late');
|
||||
f.requests[0].resolve(late);
|
||||
await flush();
|
||||
assert.equal(late.destroyCalls, 1);
|
||||
assert.deepEqual(f.host.children, [f.unrelated]);
|
||||
});
|
||||
|
||||
test('cancellation between factory resolution and mount continuation disposes the resolved view', async () => {
|
||||
const f = fixture(), controller = new AbortController();
|
||||
const result = settled(f.mount(controller.signal));
|
||||
await flush();
|
||||
const raw = f.view('settling');
|
||||
f.requests[0].resolve(raw);
|
||||
await Promise.resolve();
|
||||
controller.abort();
|
||||
await flush();
|
||||
assert.equal(result.state, 'rejected');
|
||||
assert.equal(result.value.name, 'AbortError');
|
||||
assert.equal(raw.destroyCalls, 1);
|
||||
assert.equal(f.fallbacks.length, 0);
|
||||
assert.equal(f.timers.size, 0);
|
||||
assert.deepEqual(f.host.children, [f.unrelated]);
|
||||
});
|
||||
|
||||
test('an invalid renderer API is disposed and aborted before fallback mounts', async () => {
|
||||
const f = fixture();
|
||||
const pending = f.mount();
|
||||
await flush();
|
||||
const invalid = f.view('invalid', false);
|
||||
f.requests[0].resolve(invalid);
|
||||
const fallback = await pending;
|
||||
assert.equal(fallback.kind, 'fallback');
|
||||
assert.equal(invalid.destroyCalls, 1);
|
||||
assert.equal(f.fallbacks[0].rendererAborted, true);
|
||||
assert.equal(f.timers.size, 0);
|
||||
assert.deepEqual(f.host.children, [f.unrelated, f.fallbacks[0].view.node]);
|
||||
fallback.destroy();
|
||||
});
|
||||
|
||||
test('a timed-out factory returning late cannot remove a replacement view or unrelated children', async () => {
|
||||
const f = fixture();
|
||||
const first = f.mount();
|
||||
await flush();
|
||||
f.timeout();
|
||||
const fallback = await first;
|
||||
assert.equal(fallback.kind, 'fallback');
|
||||
fallback.destroy();
|
||||
const replacementPending = f.mount();
|
||||
await flush();
|
||||
const replacement = f.view('replacement');
|
||||
f.requests[1].resolve(replacement);
|
||||
const mounted = await replacementPending;
|
||||
const late = f.view('late');
|
||||
f.requests[0].resolve(late);
|
||||
await flush();
|
||||
assert.equal(late.destroyCalls, 1);
|
||||
assert.equal(replacement.destroyCalls, 0);
|
||||
assert.equal(f.fallbacks[0].rendererAborted, true);
|
||||
assert.equal(f.timers.size, 0);
|
||||
assert.deepEqual(f.host.children, [f.unrelated, replacement.node]);
|
||||
mounted.destroy();
|
||||
});
|
||||
@@ -126,7 +126,7 @@ describe('8: the detail card never covers the selected unit\'s underframe', () =
|
||||
const { readFileSync } = await import('node:fs');
|
||||
const app = readFileSync(new URL('../../public/js/render/app.js', import.meta.url), 'utf8');
|
||||
assert.match(app, /\n {2}canvas\.addEventListener\('click', onTapTarget\);\n/, 'registered with the other canvas listeners');
|
||||
assert.match(app, /\n {6}canvas\.removeEventListener\('click', onTapTarget\);\n/, 'dropped on destroy');
|
||||
assert.match(app, /\n +canvas\.removeEventListener\('click', onTapTarget\);\n/, 'dropped on destroy');
|
||||
assert.match(app, /const onTapTarget = \(\) => \{\};/, 'a no-op: the press itself stays with the pointer events');
|
||||
});
|
||||
|
||||
|
||||
Reference in new issue
Block a user