Feature/pipeline explicit native release - #1
Conversation
Release the native GstPipeline eagerly instead of waiting for GC to finalize the wrapper, avoiding unbounded native memory growth when a pipeline is built per unit of work. dispose() drives the pipeline to NULL and drops the owning reference; a require_pipeline guard makes any use-after-dispose throw. Terminal and idempotent. Adds unit tests, README docs, and an example.
dispose() no longer issues a synchronous gst_element_set_state(NULL) on the JS thread — it just drops the owning reference via reset(), letting GStreamer tear the pipeline down when the last ref is released. Removes the event-loop blocking and the ignored state-change return raised in review. Document the stop()-and-release-elements-before-dispose contract and adjust the still-playing dispose test.
Review Target
Prior Review StatusNo prior review comments, review bodies, or inline comments on this PR. Spec Compliance: ISSUES FOUNDInterpretation Gaps
Everything else claimed is present and verified: SummaryAdds Critical Issues1. Disposing a non-NULL pipeline skips GStreamer teardown
// … When the last reference goes away GStreamer
// tears the pipeline down to NULL itself. …
pipeline.reset();Problem — upstream Major Issues2. Test asserts the unsupported sequence is safe
it("should not throw when disposing a still-playing pipeline", async () => {
…
// and GStreamer tears the pipeline down when the last reference is released.
expect(() => pipeline.dispose()).not.toThrow();
});Problem — the assertion holds only because 3. README rationale describes the superseded behaviour
climb steadily while the JS heap stays flat. `dispose()` drives the pipeline to
the NULL state and drops the native reference synchronously, so the memory is
returned immediately.Problem — Minor Issues4. The thread-safety argument is untested
Problem — the central safety claim is that in-flight workers hold their own Also noticed
Positive Observations
VerdictRequest Changes |
dispose() called pipeline.reset() without checking state. GStreamer refuses to tear down a non-NULL element, so disposing a running pipeline leaked the native memory instead of reclaiming it. Now query state and throw "dispose() requires a stopped pipeline" unless the pipeline is already NULL. Update tests to assert the still-playing case throws, add a worker-in-flight test, and correct the README and example that described the removed set_state(NULL) behaviour.
…prevents native leak)
…ndows Sending EOS to a running source and calling stop() immediately raced GStreamer's internal basesrc has_pending_eos handling, aborting the vitest worker fork on Windows CI (all JS assertions passed but the fork died). Add a waitForEos helper and await EOS on the bus before stop() in the affected EOS tests so the source streaming thread unwinds its loop cleanly before the state teardown.
Review Target
Prior Review Status
Prior report was at
Spec Compliance: ISSUES FOUNDInterpretation Gaps
Unnecessary Additions
Everything else claimed is present and verified: Summary
Critical Issues1.
|
200 × videotestsrc ! videoconvert ! queue ! fakesink |
RSS delta |
|---|---|
await play(); await stop(); dispose() |
+6.1 MB |
await play(); dispose() (still PLAYING) |
+4.5 MB |
await play(); pause(); dispose(); await pending |
+645.0 MB |
(node --expose-gc, GStreamer 1.28.6, macOS arm64; control without dispose() leaks +951.9 MB, with stop() +16.2 MB)
Problem — dispose() forces NULL (:371) then unrefs (:385), but a queued StateChangeWorker runs afterward and drives the state back up. It holds the last reference, so gst_element_dispose finalizes a non-NULL pipeline and bails out early.
Risk — a watchdog disposing a pipeline stuck in a slow play() leaks ~3 MB each, silently — the exact leak this feature exists to prevent.
Fix — track in-flight StateChangeWorkers on Pipeline; force NULL and reset() only once the count reaches zero.
Major Issues
2. Docs promise a throw that no longer exists
README.md:852
`dispose()` only drops the native reference; it does not issue a state change.
GStreamer refuses to tear down a pipeline that is not in the NULL state, so
`dispose()` throws (`dispose() requires a stopped pipeline`) if the pipeline is
still playing or paused.Problem — 8c5cb16 replaced the throw with force-to-NULL; both sentences are false, and pipeline-dispose.test.ts:23 asserts the opposite.
Risk — a caller who try/catches dispose() to detect a not-stopped pipeline gets silence, and never learns it can block the JS thread for up to 5s (pipeline.cpp:375).
Fix — state that dispose() drives the pipeline to NULL synchronously when it is not already there. examples/dispose.mjs:14 repeats the stale claim.
Minor Issues
3. Concurrent-dispose safety claim still untested
src/ts/pipeline-dispose.test.ts:40-53
const pending = pipeline.busPop(1000);
await pipeline.stop();
await pending;
expect(() => pipeline.dispose()).not.toThrow();Problem — the test added for the previous review's finding 4 awaits the worker before disposing, as its own comment says, so nothing exercises dispose-with-a-worker-in-flight — which is where finding 1 lives.
Fix — dispose while pending is still unresolved, then await it, and assert RSS does not grow across repetitions.
Also noticed
src/cpp/pipeline.cpp:355— the comment block opens "it does not issue a state change", directly above the lines that do.examples/dispose.mjs:18— says--expose-gcis needed "to also see RSS reported"; RSS prints unconditionally, the flag only affectsgc().src/ts/pipeline-eos.test.ts:16,86,99—waitForEos'sfalsereturn is discarded; a never-arriving EOS burns 10s and the test still passes.
Positive Observations
- The previous Critical is properly fixed, and measurably so: disposing a still-PLAYING pipeline now costs +4.5 MB per 200 pipelines rather than leaking. The force-to-NULL branch does what it claims.
require_pipeline()covers all ten instance methods, and every call site checks the null return rather than relying on unwinding — required underNAPI_DISABLE_CPP_EXCEPTIONS(binding.gyp:18) and easy to get wrong.- The trade-off behind force-to-NULL is argued in the code (
pipeline.cpp:355-366), including why the blocking path was accepted over throwing. waitForEosreplaces three arbitrarysetTimeout(30)sleeps with a real condition wait — a genuine flake fix rather than a longer sleep.
Verdict
Request Changes
play/pause/stop were identical except for the target state; extract a shared queue_state_change() helper so each is a one-liner. Pull the repeated timeout-parsing block into parse_timeout() (reused by bus_pop), and collapse the duplicated throw in require_pipeline() into a single condition. No behavior change.
Drop the waitForEos helper and the EOS test changes that used it, keeping this branch scoped to the dispose() feature. The Windows basesrc flake fix belongs in its own branch/PR.
2 participants
Add
Pipeline.dispose()for explicit native releaseWHAT
Adds a
dispose()method toPipelinethat releases the underlying nativeGstPipelineimmediately, plus a use-after-dispose guard, TypeScript type,documentation, an example, and unit tests.
src/cpp/pipeline.cpp/pipeline.hpp— newdispose(); drops the owningreference (
unique_ptr::reset()→gst_object_unref) without issuing a statechange. Adds a
require_pipeline()guard so every method throwsPipeline used after dispose()instead of dereferencing a freed pointer.src/ts/index.ts—dispose(): voidon thePipelineinterface.src/ts/pipeline-dispose.test.ts— coverage for idempotency and theuse-after-dispose guard (sync + async methods).
README.md/examples/dispose.mjs— usage, rationale, and the disposalcontract.
WHY
The native
GstPipelineallocation (buffers, decoders, GStreamer internals)lives outside V8's heap and is invisible to its GC accounting. A pipeline that
is simply dropped is only reclaimed when GC happens to collect the small JS
wrapper — and with a flat JS heap, V8 feels little pressure to do so. A
long-running process that builds a pipeline per unit of work (recording,
transcode, feed) can watch RSS climb steadily while the JS heap stays flat.
There was previously no way to release the native pipeline eagerly.
HOW
dispose()drops the wrapper's owning reference; GStreamer tears the pipelinedown to NULL when the last reference is released. It does not issue a
synchronous
gst_element_set_state(NULL), which would block the JS thread —callers
stop()first (the documented contract), matching howplay/pause/stopoffload transitions to a worker.BusPopWorker/StateChangeWorkerinstances each hold their own
gst_object_ref(seeasync-workers.cpp), sodropping the wrapper's reference cannot free the pipeline out from under a
running
busPop()/state change.dispose()is a no-op; any other methodcall afterward throws via the
require_pipeline()guard.stop()(orendOfStream()+ wait for EOS, thenstop()) beforedispose(), and release anygetElementByName()elements /pad-probe /
onSample()subscriptions first, since those hold independentreferences and are not invalidated by disposing the pipeline.
Testing
npm run build(native + TS) andnpm run lintclean.