Skip to content

feat(tar/unstable): add Symbol.asyncDispose to TarStreamEntry - #7192

Open
minato32 wants to merge 3 commits into
denoland:mainfrom
minato32:feat/6019-tar-async-dispose
Open

feat(tar/unstable): add Symbol.asyncDispose to TarStreamEntry#7192
minato32 wants to merge 3 commits into
denoland:mainfrom
minato32:feat/6019-tar-async-dispose

Conversation

@minato32

Copy link
Copy Markdown
Contributor

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 previous entry.readable was neither consumed nor cancelled — e.g. if (entry.path.endsWith(…)) continue; in the loop body.

This adds [Symbol.asyncDispose] to TarStreamEntry, which cancels entry.readable if it exists. Callers can then use the explicit-resource-management form to skip entries without remembering to call cancel():

for await (await using entry of stream.pipeThrough(new UntarStream())) {
  if (entry.path.endsWith(".log")) continue;
  // …consume entry.readable as usual…
}

Existing manual entry.readable?.cancel() usage keeps working unchanged; nothing else about the streaming contract changes.

@github-actions github-actions Bot added the tar label Jun 24, 2026
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.84%. Comparing base (f0431c2) to head (306cf40).
⚠️ Report is 17 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@BlackAsLight

Copy link
Copy Markdown
Contributor

While I have reservations about the using keyword, I won't stand in the way of those who want it implemented.

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 using here are around the tradeoffs it introduces. As discussed, the motivation is to prevent entry.readable from being mishandled, stopping the loop from hanging forever if someone forgets to consume or cancel it.

But using creates its own set of problems. If you want to consume the readable stream, you need to make sure you consume it entirely before the loop iterates, otherwise it gets cancelled mid-consumption. On top of that, you can't "look" for a particular file and return its readable stream as using will cancel it out from under you. Anyone who wants that needs to switch back to const and has to make sure they're properly handling entry.readable on every code path in scope, which is exactly the kind of thing using was meant to save them from.

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, const vs using, and make sure they're doing it properly.

@minato32

Copy link
Copy Markdown
Contributor Author

@BlackAsLight thanks for the review.

On Deno v1 — no skip needed. await using / Symbol.asyncDispose are supported since Deno 1.38, and async/unstable_channel_test.ts already exercises Symbol.asyncDispose in its tests under the same v1.x + canary CI matrix, so these pass there too. Locally the full tar/untar_stream_test.ts suite is green (11/11).

On the using tradeoffs — fair, and it's fully opt-in. const entry keeps today's manual-cancel behaviour unchanged; only await using triggers disposal. Anyone who needs to hold onto entry.readable (find-a-file-and-return) stays on const, exactly as now. This just gives the skip-an-entry path a safe default without forcing it on the consume path.

Comment thread tar/untar_stream.ts Outdated
* .pipeThrough(new UntarStream())
* ) {
* if (entry.path.endsWith(".log")) continue;
* // …consume entry.readable as usual…

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use regular three dots instead of the non-ascii char here?

@minato32

Copy link
Copy Markdown
Contributor Author

@bartlomieju done in 306cf40 — swapped the non-ascii ellipsis for plain ....

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tar/untar_stream.ts
* }
* ```
*/
[Symbol.asyncDispose](): Promise<void>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tar/untar_stream.ts
) + header.name,
header,
async [Symbol.asyncDispose]() {
await this.readable?.cancel();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two cases where dispose itself throws, both worth documenting since implicit disposal makes them hard to trace:

  1. If readable has errored, every cancel() on it rejects — so leaving the loop body via an exception gets you a SuppressedError wrapping both, which is confusing to read.
  2. If readable is locked (someone called getReader() without releasing, or started a pipeTo() that wasn't awaited), cancel() throws TypeError.

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.

Comment thread tar/untar_stream.ts
*/
readable?: ReadableStream<Uint8Array>;
/**
* Cancels {@linkcode TarStreamEntry.readable} if it has not already been

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread tar/untar_stream_test.ts
}
});

Deno.test("UntarStream() entry can be skipped with `await using`", async () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants