Fix State.dispose() not invoking transformer dispose for live cells (#1826)

`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 <tech.tools@boltz.bio>
This commit is contained in:
Armando Pellegrini
2026-05-09 21:17:38 +01:00
committed by GitHub
parent d7ad5a6e9f
commit 59e16e0187
4 changed files with 145 additions and 1 deletions

View File

@@ -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

View File

@@ -74,7 +74,7 @@
"js"
],
"transform": {
"\\.ts$": "esbuild-jest-transform"
"\\.ts$": ["esbuild-jest-transform", { "tsconfigRaw": "{\"compilerOptions\":{\"useDefineForClassFields\":false}}" }]
},
"moduleDirectories": [
"node_modules",

View File

@@ -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<TypeInfo>();
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<Root, Leaf, { value: number }>(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<Leaf, Leaf, {}>(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>(t: Task<T>) => 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<Root>().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<Root>().apply(A as any, { value: 1 });
builder.toRoot<Root>().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<Root, Leaf, { value: number }>(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<Root>().apply(Throwing as any, { value: 1 });
builder.toRoot<Root>().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<Root, Leaf, { value: number }>(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<Root>().apply(NoDispose as any, { value: 1 });
await state.runTask(state.updateTree(builder));
expect(() => state.dispose()).not.toThrow();
});
});

View File

@@ -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<StateTransform.Ref, StateObjectCell>).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();
}