feat: add cancellable async DICOM stream reads#502
Conversation
Add a coordinated async stream path that can abort the source when parsing stops before pixel data or after a caller-provided stop condition. Bound read-ahead so stream parsing can avoid buffering unwanted image payload when callers choose small source chunks.
Add async support for stopOnGreaterTag so metadata reads can stop when DICOM ordering shows the requested tag has been passed. Keep parent tag-stop options scoped to the current dataset level so sequence items do not trigger top-level stops.
Add shared tag normalization for parser stop options so sync and async readers accept clean, lowercase, and punctuated tag forms consistently. Invalid explicit untilTag values now fail fast instead of silently disabling early-stop behavior.
Node 24 can expose a larger pooled ArrayBuffer from fs.readFileSync().buffer for small fixtures. Use a shared test helper that slices by byteOffset and byteLength so DicomMessage reads only the fixture bytes.
Broaden async stream cancellation beyond Node destroy() by adapting Web ReadableStream sources through getReader(). Abort now calls reader.cancel(), waits for cancellation to settle, and releases reader locks while preserving existing Node stream behavior.
Only suppress abort errors that match the abort reason created by the pump. Preserve upstream failures even if parsing already reached an early-stop condition, keep generic async iterable aborts opt-in for awaiting, and cover abort-shaped source errors plus destroyOnAbort behavior.
…rly-stop-208e Support cancellable async DICOM stream reads
✅ Deploy Preview for dcmjs2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
This sounds good. @wayfarer3130 what do you think? |
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
Generally I'm ok with the changes and think they are worthwhile, but there are a few bugs:
-
Non-stream input hangs forever + unhandled rejection (high). In BufferStream.js:424, asyncSourceFromStream(stream) is called outside the try/finally, so when it throws (e.g. a caller passes an ArrayBuffer or Buffer — an easy mistake given readFile accepts buffers), setComplete() never runs. readFile() then waits forever in ensureAvailable(132) while the pump's TypeError becomes an unobserved rejection (process-fatal on modern Node). I reproduced this: readFileFromAsyncStream(new ArrayBuffer(16)) times out rather than rejecting. Fix: validate the source synchronously in pumpAsyncStream() and throw from there, before finished is created.
-
untilTag + includeUntilTagValue: true silently never stops the async reader (medium). readTagHeader only sets isUntilTag when !includeUntilTagValue (AsyncDicomReader.js:777-789), and the read loop has no post-value check equivalent to the sync reader's stop at DicomMessage.js:114. I confirmed with a test: the value is read, but stopInfo stays null and parsing continues to the end of the stream — meaning the source is never cancelled early, defeating the PR's purpose for that option combination. This predates the PR, but the PR advertises untilTag as a stream early-stop mechanism, so it should either work (add a setStopInfo check after the tag is delivered, like shouldStopAfterTag) or throw/document.
Other issues
Hot-path perf regression for all readers/writers: increment() now calls notifyReadAheadListeners() on every single byte-level read/write (BufferStream.js:362), and that method unconditionally allocates a spread copy plus a splice even when there are no listeners — which is always the case for the sync DicomMessage.readFile/write paths. Add if (this.readAheadListeners.length === 0) return; at the top of notifyReadAheadListeners.
Per-tag option normalization: readTagHeader (AsyncDicomReader.js:767) and DicomMessage._readTag (DicomMessage.js:288) spread-copy options and re-run normalizeTag for every data element, even though _read/readFile entry points already normalized once. On files with thousands of tags this is needless allocation in the hottest loop — normalize at entry points only.
destroyOnAbort: false can hang readFileFromAsyncStream: with that flag, abort() neither destroys nor calls iterator.return() (BufferStream.js:390-404), but shouldAwaitStreamAbort still defaults to awaiting for Node streams (AsyncDicomReader.js:82-91). If the pump is parked in source.next() on a stalled source (a socket, not a file), finished never settles and the read never resolves despite having succeeded. Factor destroyOnAbort into the await default, or fall back to iterator.return() when not destroying.
stopStreamOnComplete: false orphans the pump: the handle isn't exposed anywhere, so the caller can't abort it later or observe its errors — a later source error becomes an unhandled rejection. Consider storing it (this.pump = pump) or returning it in the result.
isAbortError is identity-only (BufferStream.js:451): correct for fs streams (destroy(err) re-emits the same object), but composed/pipelined Node streams can surface ERR_STREAM_PREMATURE_CLOSE instead, which would make a successful early-stopped read reject. Worth verifying against an HTTP response stream, since range-request reading is the stated use case.
shouldStop fires inside sequence items (it survives the itemOptions spread at AsyncDicomReader.js:458-464), while untilTag/stopOnGreaterTag are deliberately top-level-only. That asymmetry may be intentional, but it's undocumented and untested — a callback like the test's tagInfo.tag === TagHex.Rows would also trigger on a nested Rows.
As well, the abort mechanism should have a way to tell if this is an abort of the input because it is effectively completed the necessary read, OR it is an abort because of some external processing issue. The first one should just return the DICOM, while the second one needs to be a promise rejection. There is a DICOMweb STOW handler already written in static dicomweb that uses just such an abort capability, and it is ending up being hidden here.
|
Thanks. Looking into fixing your findings. |
Reject invalid or non-cancellable convenience sources before pumping, distinguish successful stop from external abort failure, propagate pump errors through availability waits, and correctly stop after included untilTag values. Remove per-tag option normalization work and document the cancellable Node/Web stream contract.
Wake read-ahead pumps at buffer consumption boundaries instead of every byte increment, skip post-tag async checks when no stop callback is configured, and normalize read options only at parser entry points.
Allow late external failures to reject parsing after source settlement, interrupt pending Node reads while awaiting actual teardown, preserve synchronous and delayed close errors, cancel sources after processing failures, and restore bounded-pump wakeups for all cursor movement. Clarify the read-ahead high-water-mark and stop metadata contracts.
Serialize late failures with pump finalization, normalize reasonless source errors, await Node teardown across close-event variants, restore every cursor wakeup path, validate cancellable sources before iterator acquisition, and strengthen backpressure tests so they prove source reads are actually paused.
…rly-stop-208e Harden async stream lifecycle handling
4 participants
Summary
This adds a cancellable async byte-stream path for
AsyncDicomReader.The main goal is to support metadata-only reads from large DICOM files without forcing callers to read Pixel Data. This is useful for stream/range-based workflows where the caller only needs tags before the image payload.
Changes
readFileFromAsyncStream()to coordinate stream pumping, parsing, and cleanup.untilTagstopOnGreaterTagshouldStopstopInfoso callers can inspect where parsing stopped.ReadableStreams, and generic async iterables.untilTagoption values across sync and async readers.Why this is safe
Existing ArrayBuffer parsing and manual
reader.stream.addBuffer()usage remain supported.Stream cancellation is additive:
destroy().reader.cancel()and release reader locks.iterator.return().stopOnGreaterTagfollows the DICOM rule that Data Elements are ordered by increasing tag number, and is scoped to the current dataset level so top-level stops do not trigger inside sequence items.Notes
Streams can only be stopped between delivered chunks, so callers that need strict no-pixel-byte buffering should choose source chunk/range sizes accordingly.
Deflated transfer syntax behavior is unchanged.
Checklist
pnpm test