FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: fix stream.finished on Duplex by ronag · Pull Request #33133 · nodejs/node · GitHub

/ node Public

stream: fix stream.finished on Duplex - #33133

Closed
ronag wants to merge 11 commits into
nodejs:masterfrom
nxtedition:finished-duplex
Closed

stream: fix stream.finished on Duplex#33133
ronag wants to merge 11 commits into
nodejs:masterfrom
nxtedition:finished-duplex

Conversation

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member

finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: #33130

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

finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: nodejs#33130
ronag added the stream Issues and PRs related to the stream subsystem. label Apr 28, 2020
ronag requested a review from mcollina April 28, 2020 17:38

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

@nodejs/streams @mafintosh @szmarczak

ronag commented Apr 28, 2020

Copy link
Copy Markdown
Member Author

fast-track?

ronag mentioned this pull request Apr 28, 2020
Comment thread test/parallel/test-stream-finished.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated
Comment thread test/parallel/test-stream-finished.js Outdated

Copy link
Copy Markdown
Member

Comment thread test/parallel/test-stream-finished.js Outdated

Copy link
Copy Markdown
Member

@ronag You need to delay the write to response, so the readable event is emitted after the finish one. See the gist above.

Comment thread test/parallel/test-stream-finished.js Outdated
addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 28, 2020

Copy link
Copy Markdown
Collaborator

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

(wState && wState.errorEmitted) || (rState && rState.errorEmitted) ||
(wState && wState.finished) || (rState && rState.endEmitted) ||
(rState && stream.req && stream.aborted);
const closed = (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we simplify this a bit? Perhaps at the very least group together similar dependencies, like:

const closed = (
  (wState && (wState.closed || wState.errorEmitted)) ||
  (rState && (rState.closed || rState.errorEmitted || (stream.req && stream.aborted))) ||
  (
    (!writable || (wState && wState.finished)) &&
    (!readable || (rState && rState.endEmitted))
  )
);

ronag Apr 29, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

They are grouped? See below.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unless there is a performance benefit to the above suggestion, I prefer @ronag's grouping because it's easier to read. Though it has more lines, it has less parentheses and the lines are ordered by the properties (e.g. closed) of the states. It reads like "is either state closed, or either state errorEmitted, or ..".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have not personally benchmarked it, so I cannot say if it is measurable or not. However, it is reducing the number of duplicate checks so V8 should be performing less work.

However, my code suggestion was merely one possibility. I'm not opposed to rearranging the checks in other ways, such as introducing separate if statements, etc.

ronag Apr 29, 2020
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I prefer the current formatting and believe any performance implication here would be negligible in practice. This is not a hot path as far as I'm aware. A future simplification could be to use the ?. operator.

nodejs-github-bot commented Apr 29, 2020
edited by ronag
Loading

Copy link
Copy Markdown
Collaborator

// TODO(ronag): Throw some kind of error? Does it make sense
// to call finished() on a "finished" stream?
// TODO(ronag): willEmitClose?
process.nextTick(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For bonus points this could also be simplified to process.nextTick(callback); if you want to change it while we're in here. Either way is fine though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mscdex Strangely CI fails with your suggestion. Not sure why. Leaving it as is for the purposes of this PR.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, it's because we re-assign callback in the disposer.

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

ronag commented Apr 30, 2020

Copy link
Copy Markdown
Member Author

Landed in d84f131

ronag closed this Apr 30, 2020
ronag added a commit that referenced this pull request Apr 30, 2020
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: #33130

PR-URL: #33133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request May 4, 2020
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.

Fixes: #33130

PR-URL: #33133
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos mentioned this pull request May 4, 2020
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

Labels

fast-track PRs that do not need to wait for 48 hours to land. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Async iterator does not work with Duplex streams

7 participants


Back | FazBrowse Home | New Git URL