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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 25, 2020 •
edited
Loading

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

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

@ronag
stream: don't wait for close on legacy streams
23b724a 
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 ronag mentioned this pull request Apr 25, 2020
Closed
@ronag ronag requested a review from mcollina April 25, 2020 14:48
@ronag
Copy link
Member Author

ronag commented Apr 25, 2020

adding test

@ronag
Copy link
Member Author

ronag commented Apr 25, 2020

@nodejs/streams @mafintosh

@ronag ronag added dont-land-on-v10.x stream Issues and PRs related to the stream subsystem. labels Apr 25, 2020
mcollina
mcollina approved these changes Apr 25, 2020
View reviewed changes
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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30975/

mcollina pushed a commit that referenced this pull request Apr 27, 2020
@ronag @mcollina
stream: don't wait for close on legacy streams
c15a27c 
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>
@mcollina
Copy link
Member

Landed in c15a27c

@mcollina mcollina closed this Apr 27, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@ronag @BethGriggs
stream: don't wait for close on legacy streams
f2c746d 
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>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
Merged
BethGriggs added a commit that referenced this pull request Apr 27, 2020
@BethGriggs
2020-04-28, Version 14.1.0 (Current)
f878917 
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
@BethGriggs
2020-04-28, Version 14.1.0 (Current)
3ffe68a 
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
@ronag @BethGriggs
stream: don't wait for close on legacy streams
df4518d 
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>
BethGriggs added a commit that referenced this pull request Apr 28, 2020
@BethGriggs
2020-04-28, Version 14.1.0 (Current)
9a3476e 
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 pushed a commit that referenced this pull request Apr 28, 2020
@ronag @BethGriggs
stream: don't wait for close on legacy streams
184e80a 
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>
BethGriggs added a commit that referenced this pull request Apr 28, 2020
@BethGriggs
2020-04-29, Version 14.1.0 (Current)
9706e19 
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
@BethGriggs
2020-04-29, Version 14.1.0 (Current)
9ffd8e7 
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
@BethGriggs
2020-04-29, Version 14.1.0 (Current)
49db211 
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
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Apr 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himself65 himself65 himself65 approved these changes

@targos targos targos approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Undocumented breaking change in stream.pipeline?
5 participants
@ronag @nodejs-github-bot @mcollina @targos @himself65