Skip to content

fix(yauzl): defer fd-slicer ref cleanup until stream end#40924

Closed
seia-soto wants to merge 1 commit into
microsoft:mainfrom
seia-soto:improve-yauzl-patch
Closed

fix(yauzl): defer fd-slicer ref cleanup until stream end#40924
seia-soto wants to merge 1 commit into
microsoft:mainfrom
seia-soto:improve-yauzl-patch

Conversation

@seia-soto

Copy link
Copy Markdown

fixes #40724

This is a different version to fix early-exit in node v26.1.0 happening with yauzl's fd-slicer, which respects node's stream lifecycle better rather working around with existing shape to address the issue quickly. (This is a valid point from @thejoshwolfe)

Unlike PR 168 in thejoshwolfe/yauzl, this avoids unrefing from _read() when eof is detected and avoids calling destroy() from the Writable finish handler. It also does not force autoDestroy (despite it's true by default in node v26) and let node's normal stream lifecycle invoke _destroy() when appropriate.

Lastly, using _hasRef instead of _destroyed is more clear for readers.

refs https://patch-diff.githubusercontent.com/raw/thejoshwolfe/yauzl/pull/168.diff refs thejoshwolfe/yauzl#168 (comment)


I see thejoshwolfe/yauzl#168 (comment) as a better way to address the issue in the long term. For playwright-side, these changes are fully optional but they're valuable because of two reasons. 1) the repository content might be referenced by many people and shipping better code is a responsible action 2) it has better maintainability for the future modifications.

I'm putting here for now as I cannot interact with yauzl repository but it's not responsible to keep legacy approach in my opinion. It'd be also great if someone else can contribute on behalf of me. Since the previous fix commit: c17e0b4 already fixes the issue, so there's no urgent reason to push this. I understand any decisions from the team.

This is a different version to fix early-exit in node v26.1.0, which respects node's stream lifecycle better rather working around with existing shape to address the issue quickly.

Unlike PR 168 in thejoshwolfe/yauzl, this avoids unrefing from `_read()` when eof is detected and avoids calling `destroy()` from the `Writable` finish handler. It also does not force `autoDestroy` (despite it's true by default in node v26) and let node's normal stream lifecycle invoke `_destroy()` when appropriate.

Lastly, using `_hasRef` instead of `_destroyed` is more clear for readers.

refs https://patch-diff.githubusercontent.com/raw/thejoshwolfe/yauzl/pull/168.diff
refs thejoshwolfe/yauzl#168 (comment)

---

I see thejoshwolfe/yauzl#168 (comment) as a better way to address the issue in the long term. For playwright-side, these changes are fully optional but they're valuable because of two reasons. 1) the repository content might be referenced by many people and shipping better code is a responsible action 2) it has better maintainability for the future modifications.

fixes microsoft#40724
@seia-soto

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@dgozman

dgozman commented May 20, 2026

Copy link
Copy Markdown
Collaborator

@seia-soto Thank you for the PR. We are going to wait for user reports before acting here to avoid churn.

@seia-soto

Copy link
Copy Markdown
Author

Hi @dgozman , this is a duplicate fix to be clear. No action needed for playwright as the previous fix already fixes the core cause and it doesn’t have a problem with the use case of playwright. I’m submitting better version just because I saw other projects were also picking up the patched version from playwright content. Thanks.

@seia-soto

Copy link
Copy Markdown
Author

I'm closing this version as the origin is trying to solve the issue promptly. thejoshwolfe/yauzl#169

@seia-soto seia-soto closed this May 20, 2026
@seia-soto

Copy link
Copy Markdown
Author

Playwright might be able to switch to their version once they resolve an issue.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: playwright install hangs in node v26

2 participants