feat(tar/unstable): add Symbol.asyncDispose to TarStreamEntry - #7192
feat(tar/unstable): add Symbol.asyncDispose to TarStreamEntry#7192minato32 wants to merge 3 commits into
Symbol.asyncDispose to TarStreamEntry#7192Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7192 +/- ##
==========================================
+ Coverage 94.64% 94.84% +0.19%
==========================================
Files 629 617 -12
Lines 51895 51677 -218
Branches 9373 9351 -22
==========================================
- Hits 49116 49011 -105
+ Misses 2211 2121 -90
+ Partials 568 545 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
While I have reservations about the The code itself looks fine and well implemented. I have no changes to recommend. I'm not sure whether the tests will pass in Deno v1 though, or how to tell the test runner to skip them when it's running. My main concerns with But These tradeoffs may be more desirable to everyone else here, but in my opinion it will just cause people to have two trains of thought for which one they're using, |
|
@BlackAsLight thanks for the review. On Deno v1 — no skip needed. On the |
| * .pipeThrough(new UntarStream()) | ||
| * ) { | ||
| * if (entry.path.endsWith(".log")) continue; | ||
| * // …consume entry.readable as usual… |
There was a problem hiding this comment.
can you use regular three dots instead of the non-ascii char here?
Addresses @bartlomieju's review feedback.
|
@bartlomieju done in 306cf40 — swapped the non-ascii ellipsis for plain |
bartlomieju
left a comment
There was a problem hiding this comment.
Nice — and I checked the part that actually matters, which is whether cancelling leaves the tar stream positioned correctly for the next entry. It does: #readableFile's cancel() handler drains the generator (for await (const _ of gen) {}) before releaseLock(), so cancellation consumes exactly ceil(size / 512) blocks and #buffer/#reader end up at the next header. Cancel is a drain here, not an abandon, so there's no desync — which is the whole basis for this feature working.
The ergonomic win is real too: today continue-ing past an entry hangs the loop forever, and that's a genuinely unpleasant thing to debug. Symbol.asyncDispose is the right choice given cancel() is async.
A few things to tidy before merge, inline. Also worth re-running the Deno v1.x jobs — all three were cancelled rather than passing, so @BlackAsLight's v1 support question is still formally unverified even though your 1.38 reasoning looks right to me.
| * } | ||
| * ``` | ||
| */ | ||
| [Symbol.asyncDispose](): Promise<void>; |
There was a problem hiding this comment.
Making this a required member of an exported interface is a type-level breaking change: any user code that builds a TarStreamEntry object literal — test mocks, adapters, anything wrapping the stream — stops typechecking until it adds a dispose method.
The package is 0.1.10 and @experimental, so I'm not asking you to change it. But it should be called out in the PR body so it doesn't surprise anyone at release time.
| ) + header.name, | ||
| header, | ||
| async [Symbol.asyncDispose]() { | ||
| await this.readable?.cancel(); |
There was a problem hiding this comment.
Two cases where dispose itself throws, both worth documenting since implicit disposal makes them hard to trace:
- If
readablehas errored, everycancel()on it rejects — so leaving the loop body via an exception gets you aSuppressedErrorwrapping both, which is confusing to read. - If
readableis locked (someone calledgetReader()without releasing, or started apipeTo()that wasn't awaited),cancel()throwsTypeError.
The fully-consumed and double-dispose cases are both fine — cancel() on a closed stream is a resolved no-op per spec — so it's only these two. A sentence in the JSDoc would be enough; swallowing them would also be defensible for a dispose method, but I'd rather that were a deliberate choice than a silent one.
| */ | ||
| readable?: ReadableStream<Uint8Array>; | ||
| /** | ||
| * Cancels {@linkcode TarStreamEntry.readable} if it has not already been |
There was a problem hiding this comment.
This JSDoc is the only place the new capability is documented, but it's not where people look. The class-level "### Usage" note just above (line ~150 in the original) and tar/mod.ts both still say only "consume or cancel" — so a reader following the module docs will never learn that await using exists.
Could you add a line to both? The continue-past-an-entry case is common enough that it deserves a mention where the contract is stated.
| } | ||
| }); | ||
|
|
||
| Deno.test("UntarStream() entry can be skipped with `await using`", async () => { |
There was a problem hiding this comment.
This asserts the set of paths seen, which proves the loop didn't hang — but not that the stream is still correctly positioned. If disposal consumed the wrong number of blocks, you'd still see the right paths while the contents silently shifted.
Asserting the bytes of an entry that follows a skipped one would pin the actual invariant. Three more cases worth covering while you're in here: double dispose, dispose after the readable was fully consumed, and dispose while the readable is locked (see my note on the implementation).
Refs #6019.
The most common footgun with
UntarStream(raised by @lowlighter, @dsherret, and others on the issue) is that requesting the next entry hangs forever if the previousentry.readablewas neither consumed nor cancelled — e.g.if (entry.path.endsWith(…)) continue;in the loop body.This adds
[Symbol.asyncDispose]toTarStreamEntry, which cancelsentry.readableif it exists. Callers can then use the explicit-resource-management form to skip entries without remembering to callcancel():Existing manual
entry.readable?.cancel()usage keeps working unchanged; nothing else about the streaming contract changes.