fix(yauzl): defer fd-slicer ref cleanup until stream end#40924
Open
seia-soto wants to merge 1 commit into
Open
fix(yauzl): defer fd-slicer ref cleanup until stream end#40924seia-soto wants to merge 1 commit into
seia-soto wants to merge 1 commit into
Conversation
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
Author
|
@microsoft-github-policy-service agree |
Collaborator
|
@seia-soto Thank you for the PR. We are going to wait for user reports before acting here to avoid churn. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 callingdestroy()from theWritablefinish handler. It also does not forceautoDestroy(despite it's true by default in node v26) and let node's normal stream lifecycle invoke_destroy()when appropriate.Lastly, using
_hasRefinstead of_destroyedis 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.