Skip to content

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

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

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

Conversation

@minato32

Copy link
Copy Markdown

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.56%. Comparing base (cdf74a8) to head (2013d64).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7192      +/-   ##
==========================================
- Coverage   94.57%   94.56%   -0.01%     
==========================================
  Files         636      637       +1     
  Lines       52142    52153      +11     
  Branches     9401     9402       +1     
==========================================
+ Hits        49315    49321       +6     
- Misses       2249     2254       +5     
  Partials      578      578              

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

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.

2 participants