Undocumented breaking change in stream.pipeline? #33050

aduh95 · 2020-04-24T23:14:09Z

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

The text was updated successfully, but these errors were encountered:

himself65 · 2020-04-25T01:11:45Z

I think we have another undocumented semver-major change

like #32987

ronag · 2020-04-25T07:42:52Z

#32987

@himself65 how is that related to streams?

ronag · 2020-04-25T07:43:20Z

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

himself65 · 2020-04-25T07:52:39Z

@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

ronag · 2020-04-25T08:05:45Z

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

targos · 2020-04-25T08:07:30Z

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

ronag · 2020-04-25T08:08:54Z

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.

ronag · 2020-04-25T08:13:22Z

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

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

targos · 2020-04-25T08:46:32Z

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

aduh95 · 2020-04-25T09:38:10Z

@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

ronag · 2020-04-25T11:22:42Z

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

ronag · 2020-04-25T14:35:52Z

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.

ronag · 2020-04-25T14:38:24Z

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

jasnell · 2020-04-25T14:39:16Z

/cc @mafintosh @mcollina

Try to detect non standard streams and don't wait for 'close' on these. In particular if we detected that destroyed is true before we expect it to be then fallback to compat behavior. Fixes: nodejs#33050

ronag · 2020-04-25T14:48:15Z

I think this should fix it, #33058

mcollina · 2020-04-25T18:21:27Z

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

ronag · 2020-04-25T18:30:53Z

@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

Try to detect non standard streams and don't wait for 'close' on these. In particular if we detected that destroyed is true before we expect it to be then fallback to compat behavior. Fixes: #33050 PR-URL: #33058 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

barisusakli · 2020-04-29T22:30:17Z

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

aduh95 · 2020-04-29T22:33:59Z

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

StephenLynx · 2020-05-25T17:38:21Z

14.3 and multi-party is still broken tbh fam

exe-dealer · 2020-09-28T10:49:37Z

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

himself65 added the stream Issues and PRs related to the stream subsystem. label Apr 25, 2020

ronag mentioned this issue Apr 25, 2020

stream: don't wait for close on legacy streams #33058

Closed

4 tasks

barisusakli mentioned this issue Apr 25, 2020

req.files empty on nodejs 14 expressjs/connect-multiparty#29

Closed

This comment has been minimized.

Sign in to view

papb mentioned this issue Apr 26, 2020

Sequelize misbehaving with Node v14 sequelize/sequelize#12158

Closed

mcollina closed this as completed in c15a27c Apr 27, 2020

arjunattam mentioned this issue Oct 1, 2020

[BUG] Failed to launch chromium because executable doesn't exist microsoft/playwright#4033

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Undocumented breaking change in stream.pipeline? #33050

Undocumented breaking change in stream.pipeline? #33050

aduh95 commented Apr 24, 2020

himself65 commented Apr 25, 2020

ronag commented Apr 25, 2020

ronag commented Apr 25, 2020

himself65 commented Apr 25, 2020

ronag commented Apr 25, 2020

targos commented Apr 25, 2020

ronag commented Apr 25, 2020

ronag commented Apr 25, 2020 •

edited

Loading

targos commented Apr 25, 2020 •

edited

Loading

aduh95 commented Apr 25, 2020

ronag commented Apr 25, 2020 •

edited

Loading

ronag commented Apr 25, 2020 •

edited

Loading

ronag commented Apr 25, 2020

jasnell commented Apr 25, 2020

ronag commented Apr 25, 2020

mcollina commented Apr 25, 2020

ronag commented Apr 25, 2020 •

edited

Loading

This comment has been minimized.

barisusakli commented Apr 29, 2020

aduh95 commented Apr 29, 2020

StephenLynx commented May 25, 2020

exe-dealer commented Sep 28, 2020

Undocumented breaking change in stream.pipeline? #33050

Undocumented breaking change in stream.pipeline? #33050

Comments

aduh95 commented Apr 24, 2020

What steps will reproduce the bug?

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

What is the expected behavior?

What do you see instead?

Additional information

himself65 commented Apr 25, 2020

ronag commented Apr 25, 2020

ronag commented Apr 25, 2020

himself65 commented Apr 25, 2020

ronag commented Apr 25, 2020

targos commented Apr 25, 2020

ronag commented Apr 25, 2020

ronag commented Apr 25, 2020 • edited Loading

targos commented Apr 25, 2020 • edited Loading

aduh95 commented Apr 25, 2020

ronag commented Apr 25, 2020 • edited Loading

ronag commented Apr 25, 2020 • edited Loading

ronag commented Apr 25, 2020

jasnell commented Apr 25, 2020

ronag commented Apr 25, 2020

mcollina commented Apr 25, 2020

ronag commented Apr 25, 2020 • edited Loading

This comment has been minimized.

barisusakli commented Apr 29, 2020

aduh95 commented Apr 29, 2020

StephenLynx commented May 25, 2020

exe-dealer commented Sep 28, 2020

ronag commented Apr 25, 2020 •

edited

Loading

targos commented Apr 25, 2020 •

edited

Loading

ronag commented Apr 25, 2020 •

edited

Loading

ronag commented Apr 25, 2020 •

edited

Loading

ronag commented Apr 25, 2020 •

edited

Loading