Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: pipeline should only destroy un-finished streams. #32968

Closed
wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 21, 2020

This PR logically reverts #31940 which
has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

stream.pipeline() will call stream.destroy(err) on all streams except:

  • Readable streams which have emitted 'end' or 'close'.
  • Writable streams which have emitted 'finish' or 'close'.

The behavior introduced in #31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: #32954
Fixes: #32955

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This PR logically reverts nodejs#31940 which
has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

`stream.pipeline()` will call `stream.destroy(err)` on all streams except:
  * `Readable` streams which have emitted `'end'` or `'close'`.
  * `Writable` streams which have emitted `'finish'` or `'close'`.

The behavior introduced in nodejs#31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: nodejs#32954
Fixes: nodejs#32955
@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

@nodejs/streams: I'm sorry about the pain caused by #31940.

@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

@mafintosh Could you take a look at this and see if it resolves the issues you have encountered?

@ronag ronag added stream Issues and PRs related to the stream subsystem. v13.x labels Apr 21, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but we should test this with the offending modules.

cc @mafintosh

@mcollina
Copy link
Member

which versions should this backported to v13 and v14?

@vweevers
Copy link
Contributor

Should we add tar-stream to citgm?

@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

which versions should this backported to v13 and v14?

v13 and v14

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added the v14.x label Apr 21, 2020
@mafintosh
Copy link
Member

@ronag testing (my macbook isn't the fastest at compiling v8). also streams are tricky, and i appreciate you maintaining all this :)

@mafintosh
Copy link
Member

Confirmed this fixes the tar-stream regression.

@mafintosh
Copy link
Member

Don't know what the release schedule is like but can this land before 14 goes out?

@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

Don't know what the release schedule is like but can this land before 14 goes out?

I think it's too late. v14 just got merged.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

I'm calling for a fast-track here, and get it into 14.0.1 when it gets out.

cc @nodejs/tsc

@ronag

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 21, 2020

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 21, 2020

ronag added a commit to nxtedition/node that referenced this pull request Apr 23, 2020
This PR logically reverts nodejs#31940 which
has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

`stream.pipeline()` will call `stream.destroy(err)` on all streams except:
  * `Readable` streams which have emitted `'end'` or `'close'`.
  * `Writable` streams which have emitted `'finish'` or `'close'`.

The behavior introduced in nodejs#31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: nodejs#32954
Fixes: nodejs#32955

PR-URL: nodejs#32968
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
Backport-PR-URL: nodejs#32980
ronag added a commit to nxtedition/node that referenced this pull request Apr 23, 2020
An unfortunate overlap between two PR that by themselves pass
CI but together pass a test.

nodejs#32967 changes so that
pipeline does not wait for 'close'.

nodejs#32968 changed so that
all streams are not destroyed.

Which made one test fail when expected the stream to be
destroyed during pipeline callback.
@ronag ronag mentioned this pull request Apr 23, 2020
4 tasks
BridgeAR pushed a commit that referenced this pull request Apr 23, 2020
An unfortunate overlap between two PR that by themselves pass
CI but together pass a test.

#32967 changes so that
pipeline does not wait for 'close'.

#32968 changed so that
all streams are not destroyed.

Which made one test fail when expected the stream to be
destroyed during pipeline callback.

PR-URL: #33030
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
This PR logically reverts #31940
which has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

`stream.pipeline()` will call `stream.destroy(err)` on all streams
except:
  * `Readable` streams which have emitted `'end'` or `'close'`.
  * `Writable` streams which have emitted `'finish'` or `'close'`.

The behavior introduced in #31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: #32954
Fixes: #32955

PR-URL: #32968
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
An unfortunate overlap between two PR that by themselves pass
CI but together pass a test.

#32967 changes so that
pipeline does not wait for 'close'.

#32968 changed so that
all streams are not destroyed.

Which made one test fail when expected the stream to be
destroyed during pipeline callback.

PR-URL: #33030
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BethGriggs added a commit that referenced this pull request Apr 27, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- module: do not warn when accessing `\_\_esModule` of unfinished exports (Anna Henningsen)
  [#33048](#33048)
- stream:
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)

PR-URL: #33103
BethGriggs added a commit that referenced this pull request Apr 28, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- module: do not warn when accessing `\_\_esModule` of unfinished exports (Anna Henningsen)
  [#33048](#33048)
- stream:
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)

PR-URL: #33103
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
This PR logically reverts #31940
which has caused lots of unnecessary breakage in the ecosystem.

This PR also aligns better with the actual documented behavior:

`stream.pipeline()` will call `stream.destroy(err)` on all streams
except:
  * `Readable` streams which have emitted `'end'` or `'close'`.
  * `Writable` streams which have emitted `'finish'` or `'close'`.

The behavior introduced in #31940
was much more aggressive in terms of destroying streams. This was
good for avoiding potential resources leaks however breaks some
common assumputions in legacy streams.

Furthermore, it makes the code simpler and removes some hacks.

Fixes: #32954
Fixes: #32955

PR-URL: #32968
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
An unfortunate overlap between two PR that by themselves pass
CI but together pass a test.

#32967 changes so that
pipeline does not wait for 'close'.

#32968 changed so that
all streams are not destroyed.

Which made one test fail when expected the stream to be
destroyed during pipeline callback.

PR-URL: #33030
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs added a commit that referenced this pull request Apr 28, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- doc: add juanarbol as collaborator (Juan José Arboleda)
  [#32906](#32906)
- http: doc deprecate abort and improve docs (Robert Nagy)
  [#32807](#32807)
- module: do not warn when accessing `__esModule` of unfinished exports
  (Anna Henningsen) [#33048](#33048)
- n-api: detect deadlocks in thread-safe function (Gabriel Schulhof)
  [#32860](#32860)
- src: deprecate embedder APIs with replacements (Anna Henningsen)
  [#32858](#32858)
- stream:
  - don't emit end after close (Robert Nagy)
    [#33076](#33076)
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)

PR-URL: #33103
BethGriggs added a commit that referenced this pull request Apr 28, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- doc: add juanarbol as collaborator (Juan José Arboleda)
  [#32906](#32906)
- http: doc deprecate abort and improve docs (Robert Nagy)
  [#32807](#32807)
- module: do not warn when accessing `__esModule` of unfinished exports
  (Anna Henningsen) [#33048](#33048)
- n-api: detect deadlocks in thread-safe function (Gabriel Schulhof)
  [#32860](#32860)
- src: deprecate embedder APIs with replacements (Anna Henningsen)
  [#32858](#32858)
- stream:
  - don't emit end after close (Robert Nagy)
    [#33076](#33076)
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)
- vm: add importModuleDynamically option to compileFunction (Gus Caplan)
  [#32985](#32985)

PR-URL: #33103
BethGriggs added a commit that referenced this pull request Apr 29, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- doc: add juanarbol as collaborator (Juan José Arboleda)
  [#32906](#32906)
- http: doc deprecate abort and improve docs (Robert Nagy)
  [#32807](#32807)
- module: do not warn when accessing `__esModule` of unfinished exports
  (Anna Henningsen) [#33048](#33048)
- n-api: detect deadlocks in thread-safe function (Gabriel Schulhof)
  [#32860](#32860)
- src: deprecate embedder APIs with replacements (Anna Henningsen)
  [#32858](#32858)
- stream:
  - don't emit end after close (Robert Nagy)
    [#33076](#33076)
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)
- vm: add importModuleDynamically option to compileFunction (Gus Caplan)
  [#32985](#32985)

PR-URL: #33103
BethGriggs added a commit that referenced this pull request Apr 29, 2020
Notable changes:

- deps: upgrade openssl sources to 1.1.1g (Hassaan Pasha)
  [#32971](#32971)
- doc: add juanarbol as collaborator (Juan José Arboleda)
  [#32906](#32906)
- http: doc deprecate abort and improve docs (Robert Nagy)
  [#32807](#32807)
- module: do not warn when accessing `__esModule` of unfinished exports
  (Anna Henningsen) [#33048](#33048)
- n-api: detect deadlocks in thread-safe function (Gabriel Schulhof)
  [#32860](#32860)
- src: deprecate embedder APIs with replacements (Anna Henningsen)
  [#32858](#32858)
- stream:
  - don't emit end after close (Robert Nagy)
    [#33076](#33076)
  - don't wait for close on legacy streams (Robert Nagy)
    [#33058](#33058)
  - pipeline should only destroy un-finished streams (Robert Nagy)
    [#32968](#32968)
- vm: add importModuleDynamically option to compileFunction (Gus Caplan)
  [#32985](#32985)

PR-URL: #33103
Pizzacus referenced this pull request in suisei-cn/hosimati.suisei.moe May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplex stream pipeline regression in 13 tar-stream regression in 13 with pipeline
6 participants