From 59e16e018726857a1ec4caf6b3033ba7a0a0b612 Mon Sep 17 00:00:00 2001 From: Armando Pellegrini <84039859+armyalpaca@users.noreply.github.com> Date: Sat, 9 May 2026 21:17:38 +0100 Subject: [PATCH] Fix `State.dispose()` not invoking transformer dispose for live cells (#1826) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Transformer.Definition.dispose` is documented as "automatically called on deleting an object," but `State.dispose()` only disposed its own event subjects and action manager — it never iterated still-live cells to call their per-transformer dispose. Cells holding GL buffers, mesh data, etc. only had their dispose fired on explicit deletion (e.g. `clear()`), so any consumer that called `plugin.dispose()` without first awaiting `plugin.clear()` retained the callback chain, the GL buffers it points at, and any closures captured by it. In a long-running single-page app where the user navigates between routes that mount/unmount a Mol* viewer, this leaked roughly 25–50 MB of process RSS per cycle even with `plugin.dispose()` correctly called. A 20-cycle E2E mount/unmount harness on a 1AKE structure measured a +541 MB RSS / +266 MB JS-heap delta in the unconditional-`dispose()` case; calling `await plugin.clear()` before `plugin.dispose()` halved the residual leak, confirming the per-cell dispose path was missing on the unconditional `dispose()` route. This change walks the cell tree once (post-order via the existing `StateTree.doPostOrder` helper) and invokes the per-transformer dispose for every still-live cell, swallowing+warning on errors so a single faulty transformer can't prevent siblings from cleaning up. The existing per-cell `dispose` helper is reused for consistency with `updateNode`/`findDeletes` semantics. Tests cover: chained transformers, sibling subtrees, throwing-dispose isolation, and transformers without a dispose definition. Also adds `useDefineForClassFields: false` to the jest esbuild transform so tests can construct `State` (the `TransientTree` parameter property + class field pattern relies on legacy class-field semantics, which `tsc` honors via `target: es2018` but esbuild's default `esnext` target does not). Fixes #1825 Co-authored-by: Armando Pellegrini --- CHANGELOG.md | 1 + package.json | 2 +- src/mol-state/_spec/state.spec.ts | 126 ++++++++++++++++++++++++++++++ src/mol-state/state.ts | 17 ++++ 4 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 src/mol-state/_spec/state.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dd345ac9..08b738c03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Note that since we don't clearly distinguish between a public and private interf ## [Unreleased] - Fix empty transforms default in `ShapeFromPly` +- Fix memory leak in `State.dispose()` not invoking transformer `dispose` callbacks for live cells ## [v5.9.0] - 2026-05-03 - Fix edge case when `PluginSpec.animations` is empty diff --git a/package.json b/package.json index 19c42d62a..39b3071e5 100644 --- a/package.json +++ b/package.json @@ -74,7 +74,7 @@ "js" ], "transform": { - "\\.ts$": "esbuild-jest-transform" + "\\.ts$": ["esbuild-jest-transform", { "tsconfigRaw": "{\"compilerOptions\":{\"useDefineForClassFields\":false}}" }] }, "moduleDirectories": [ "node_modules", diff --git a/src/mol-state/_spec/state.spec.ts b/src/mol-state/_spec/state.spec.ts new file mode 100644 index 000000000..d9a151fa0 --- /dev/null +++ b/src/mol-state/_spec/state.spec.ts @@ -0,0 +1,126 @@ +/** + * Copyright (c) 2026 mol* contributors, licensed under MIT, See LICENSE file for more info. + */ + +import { State, StateObject, StateTransformer } from '../../mol-state'; +import { Task } from '../../mol-task'; + +interface TypeInfo { name: string; typeClass: 'Root' | 'Data' } +const Create = StateObject.factory(); + +class Root extends Create({ name: 'Root', typeClass: 'Root' }) { } +class Leaf extends Create<{ value: number }>({ name: 'Leaf', typeClass: 'Data' }) { } + +const NS = 'state-dispose-spec'; +let counter = 0; + +function leafTransformer(spy: () => void) { + return StateTransformer.create(NS, { + name: `create-leaf-${counter++}`, + from: [Root], + to: [Leaf], + display: { name: 'Create Leaf' }, + params: () => ({} as any), + apply({ params }) { return new Leaf({ value: params.value }); }, + dispose() { spy(); } + }); +} + +function chainedTransformer(spy: () => void) { + return StateTransformer.create(NS, { + name: `chained-leaf-${counter++}`, + from: [Leaf], + to: [Leaf], + display: { name: 'Chained Leaf' }, + apply({ a }) { return new Leaf({ value: a.data.value + 1 }); }, + dispose() { spy(); } + }); +} + +function newState() { + return State.create(new Root({}), { runTask: (t: Task) => t.run() }); +} + +describe('State.dispose', () => { + it('calls transformer.dispose for every live cell', async () => { + const leafSpy = jest.fn(); + const chainSpy = jest.fn(); + const A = leafTransformer(leafSpy); + const B = chainedTransformer(chainSpy); + + const state = newState(); + const builder = state.build(); + builder.toRoot().apply(A as any, { value: 1 }).apply(B as any, {}); + await state.runTask(state.updateTree(builder)); + + // root + 2 transformer outputs. + expect(state.cells.size).toBe(3); + + state.dispose(); + + expect(leafSpy).toHaveBeenCalledTimes(1); + expect(chainSpy).toHaveBeenCalledTimes(1); + }); + + it('disposes all sibling subtrees', async () => { + const spyA = jest.fn(); + const spyB = jest.fn(); + const A = leafTransformer(spyA); + const B = leafTransformer(spyB); + + const state = newState(); + const builder = state.build(); + builder.toRoot().apply(A as any, { value: 1 }); + builder.toRoot().apply(B as any, { value: 2 }); + await state.runTask(state.updateTree(builder)); + + state.dispose(); + + expect(spyA).toHaveBeenCalledTimes(1); + expect(spyB).toHaveBeenCalledTimes(1); + }); + + it('does not throw when a transformer dispose throws', async () => { + const goodSpy = jest.fn(); + const Throwing = StateTransformer.create(NS, { + name: `throwing-leaf-${counter++}`, + from: [Root], + to: [Leaf], + display: { name: 'Throwing Leaf' }, + apply({ params }) { return new Leaf({ value: params.value }); }, + dispose() { throw new Error('boom'); } + }); + const Good = leafTransformer(goodSpy); + + const state = newState(); + const builder = state.build(); + builder.toRoot().apply(Throwing as any, { value: 1 }); + builder.toRoot().apply(Good as any, { value: 2 }); + await state.runTask(state.updateTree(builder)); + + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + try { + expect(() => state.dispose()).not.toThrow(); + } finally { + warn.mockRestore(); + } + expect(goodSpy).toHaveBeenCalledTimes(1); + }); + + it('is a no-op for transformers without a dispose definition', async () => { + const NoDispose = StateTransformer.create(NS, { + name: `no-dispose-${counter++}`, + from: [Root], + to: [Leaf], + display: { name: 'No-dispose Leaf' }, + apply({ params }) { return new Leaf({ value: params.value }); } + }); + + const state = newState(); + const builder = state.build(); + builder.toRoot().apply(NoDispose as any, { value: 1 }); + await state.runTask(state.updateTree(builder)); + + expect(() => state.dispose()).not.toThrow(); + }); +}); diff --git a/src/mol-state/state.ts b/src/mol-state/state.ts index 4c93c74d6..fede7bc2b 100644 --- a/src/mol-state/state.ts +++ b/src/mol-state/state.ts @@ -159,6 +159,23 @@ class State { } dispose() { + // Dispose every still-live cell so transformer dispose callbacks + // (e.g. WebGL/GL buffer cleanup) actually run. Without this, + // calling dispose() on a State that still has cells leaks any + // resources held by transformer dispose callbacks because they + // would only fire on per-cell deletion (see updateNode/findDeletes). + const refs: StateTransform.Ref[] = []; + StateTree.doPostOrder(this._tree, this._tree.root, { refs }, (n, _, s) => { s.refs.push(n.ref); }); + for (let i = refs.length - 1; i >= 0; i--) { + const cell = (this.cells as Map).get(refs[i]); + if (!cell) continue; + try { + dispose(cell.transform, cell.obj, cell.transform.params, cell.cache, this.globalContext); + } catch (e) { + console.warn('Error in transformer dispose during State.dispose', e); + } + } + this.ev.dispose(); this.actions.dispose(); }