Skip to content

Undocumented breaking change in stream.pipeline? #33050

Closed
Closed
Undocumented breaking change in stream.pipeline?#33050
Labels
streamIssues and PRs related to Node.js streams.

Description

@aduh95
aduh95
opened on Apr 24, 2020
  • Version: 14.0.0
  • Platform: any
  • Subsystem: stream, doc

What steps will reproduce the bug?

It seems Node.js 14 introduced a breaking change in the stream.pipeline API, however the docs doesn't reference any change related to Node 14.

Refs: max-mapper/extract-zip#94

TLDR of the above issue is that sometimes the promisified version of pipeline never resolves, causing the program to hang or to exit with unfinished promises.

How often does it reproduce? Is there a required condition?

I have a limited knowledge of the Node.js Stream API, I haven't been able to isolate the bug in the issue I referenced above.

What is the expected behavior?

If there is a breaking change on Node.js 14, it should be documented, at least in the YAML metadata.

Ideally there would be a migration guide to workaround the breaking change.

What do you see instead?

Last documented change references 13.10.0.

Additional information

Probably related to #32158.

CC: @ronag

Activity

  1. himself65
    added
    streamIssues and PRs related to Node.js streams.
    on Apr 25, 2020

  2. himself65 commented on Apr 25, 2020

    @himself65
    himself65
    on Apr 25, 2020
    Member

    I think we have another undocumented semver-major change

    like #32987

  3. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    #32987

    @himself65 how is that related to streams?

  4. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    @aduh95 Do you think you could provide a full example on how to reproduce?

  5. himself65 commented on Apr 25, 2020

    @himself65
    himself65
    on Apr 25, 2020
    Member

    @himself65 how is that related to streams?

    Absolutely not, I just thought this's maybe an undocumented update issue like that one. Because the commit 1428a92 which author mentioned to only release in V14

  6. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    FYI, I've moved the conversation to max-mapper/extract-zip#94 for now until I know how to reproduce the issue.

  7. targos commented on Apr 25, 2020

    @targos
    targos
    on Apr 25, 2020
    Member

    I had a similar issue with another zip extracting library. I can try to make a repro.

  8. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    I had a similar issue with another zip extracting library. I can try to make a repro.

    Thanks. I'm struggling to make it fail. So far all my tries succeed.

  9. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    Ok, I managed to make it fail, pre #32967.

    @aduh95 Can you confirm whether this is still a problem after #32967?

  10. targos commented on Apr 25, 2020

    @targos
    targos
    on Apr 25, 2020
    Member

    @ronag see https://github.com/targos/bug-zip-pipeline

    Expected: node test.js should print two lines (start/finished reading zip file) and the zip file should be fully extracted at result/.

    Actual:

    • Node.js 13: works
    • Node.js 14.0.0: only one file is extracted and one line is printed
    • Latest 15.0.0 nightly: only one file is extracted and one line is printed

    Edit:

    If pipeline is replaced with manual pipe + on('finish'), it works in all versions.

  11. aduh95 commented on Apr 25, 2020

    @aduh95
    aduh95
    on Apr 25, 2020
    ContributorAuthor

    @aduh95 Can you confirm whether this is still a problem after #32967?

    Yes, I can confirm it is still a problem. I am also able to reproduce using @targos repo:

    $ npm install
$ ../node/out/Release/node --version
v15.0.0-pre
$ ../node/out/Release/node test.js
start reading zip file
$ node13 --version
v13.13.0
$ node13 test.js 
(node:53994) ExperimentalWarning: The ESM module loader is experimental.
start reading zip file
finished reading zip file

  12. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    The problem seems to be that the read stream never emits a 'close' event event though it probably should. Either need to fix the read stream or make finished even more strict in regards to when to assume it should wait for 'close'. Digging...

  13. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    So the problem goes back to fd-slicer which implements its own destroy() function overriding the stream default autoDestroy behavior, thus it never emits 'close'.

    https://github.com/andrewrk/node-fd-slicer/blob/master/index.js#L102

    At the moment I'm a little unsure how to fix this without basically to large degree disabling the willEmitClose behavior https://github.com/nodejs/node/blob/master/lib/internal/streams/end-of-stream.js#L75.

  14. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    Neither fd-slicer not yauzl seems to be maintained anymore 😞 so fixing them does not feel likely. thejoshwolfe/yauzl#115

  15. jasnell commented on Apr 25, 2020

    @jasnell
    jasnell
    on Apr 25, 2020
    Member

    /cc @mafintosh @mcollina

  16. ronag
    added a commit that references this issue on Apr 25, 2020

    stream: don't wait for close on legacy streams

    23b724a

  17. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    I think this should fix it, #33058

  18. mcollina commented on Apr 25, 2020

    @mcollina
    mcollina
    on Apr 25, 2020
    SponsorMember

    @ronag could you also send a PR that list the semver-major changes to the history of pipeline/finished etc?

  19. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020
    Member

    @ronag could you also send a PR that list the semver-major changes to the history of pipeline/finished etc?

    Sure, though I don't quite understand what that means in practice? In the documentation? Where and how? i.e. what is "the history of pipeline/finished etc"?

    #33065

  20. ronag commented on Apr 25, 2020

    @ronag
    ronag
    on Apr 25, 2020 · Hidden as outdated
  21. mcollina
    closed this as completedin c15a27con Apr 27, 2020
  22. BethGriggs
    added 3 commits that reference this issue on Apr 27, 2020

    stream: don't wait for close on legacy streams

    f2c746d

    stream: don't wait for close on legacy streams

    184e80a

  23. barisusakli commented on Apr 29, 2020

    @barisusakli
    barisusakli
    on Apr 29, 2020

    Can anyone confirm if this issue is fixed? File uploads still fail with a simple app with connect-multiparty. expressjs/connect-multiparty#29

  24. aduh95 commented on Apr 29, 2020

    @aduh95
    aduh95
    on Apr 29, 2020
    ContributorAuthor

    I can confirm it is fixed on Node.js 14.1.0, I haven't tested the app you referenced though.

  25. StephenLynx commented on May 25, 2020

    @StephenLynx
    StephenLynx
    on May 25, 2020

    14.3 and multi-party is still broken tbh fam

  26. exedealer commented on Sep 28, 2020

    @exedealer
    exedealer
    on Sep 28, 2020

    here is my minimal bug reproduction https://github.com/exe-dealer/node-iss-33050
    node v10 works fine, but node v14 is broken

  27. sjarmak
    added a commit that references this issue on Jul 27, 2026

    stream: don't wait for close on legacy streams

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    streamIssues and PRs related to Node.js streams.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions