stream: finished should invoke callback for closed streams by ronag · Pull Request #31509 · nodejs/node

ronag · 2020-01-25T16:33:18Z

Previously finished(stream, cb) would not invoke the callback for streams
that have already finished, ended or errored before being
passed to finished(stream, cb).

Ref: #31508
Ref: #31314 (comment)

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

mcollina

Can you add a few tests with autoDestroy: false and autoClose: false? I know these are edge/possibly legacy cases in v14, but it's important we document that behavior as well.

ronag · 2020-01-25T16:54:48Z

@mcollina: good call, updated

lpinca · 2020-01-25T16:57:08Z

I don't know. In my opinion passing an already "finished" stream to finished() is a usage error so I think it should throw instead of calling the callback.

mcollina · 2020-01-25T17:00:03Z

I think throwing would be bad for usability, I would rather call the callback with an error.

I'm ok in both erroring or not erroring as long as something happens.

ronag · 2020-01-25T17:04:12Z

I’m good with send error into callback as well. @mcollina that would break some existing use cases, such as the one in your revert PR, i.e. Async iterating a destroyed stream.

ronag · 2020-01-25T17:05:24Z

Though it would make things more difficult. Because we have to distinguish between a closed and a closing stream. Not sure that is possible in a safe way considering compat.

lpinca · 2020-01-25T17:18:09Z

I think throwing is the right thing to do from an API consistency and logical point of view but I'm fine either way. It doesn't make much sense to me to pass an already "finished" stream to finished().

ronag · 2020-01-25T19:47:48Z

I think throwing is the right thing to do from an API consistency and logical point of view but I'm fine either way. It doesn't make much sense to me to pass an already "finished" stream to finished().

The problem is that it's quite difficult currently for users to determine whether a stream is "finished" and what that exactly means. Though I agree some kind of error would be better but I think it's a bit more complicated and therefore I would prefer to land this PR as is. My understanding is that neither of you are opposed to this?

mcollina · 2020-01-25T23:31:10Z

I’m not opposed.

ronag · 2020-01-26T07:20:24Z

This needs another TSC approval. @jasnell @addaleax?

nodejs-github-bot · 2020-01-26T07:27:34Z

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

ronag · 2020-01-26T17:28:06Z

Rebased to fix conflict.

nodejs-github-bot · 2020-01-26T17:28:42Z

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

nodejs-github-bot · 2020-01-26T21:52:19Z

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

Trott · 2020-01-27T05:47:37Z

I’m not opposed.

@mcollina Does this mean your change request can be cleared?

Trott · 2020-01-27T05:48:22Z

Removing author ready until this gets at least one approval.

mcollina

lgtm

ronag · 2020-01-29T08:05:46Z

I would prefer to know what is the fix for spawn-wrap and if it's feasible to support both < 14 and 14.

I can't reproduce the async-wrap issue on MacOS.

I would expect spawn-wrap to fail on multiple platforms if it were affected by this. I think it's OK.

Seems to fail on Debian.

nodejs-github-bot · 2020-01-31T20:13:53Z

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

ronag · 2020-01-31T20:48:13Z

CI passes

ronag · 2020-01-31T21:14:48Z

spawn-wrap flaky issue #31596

ronag · 2020-02-01T20:09:35Z

@Trott: Opinon on CITGM? Can this land?

Trott · 2020-02-05T19:24:39Z

@Trott: Opinon on CITGM? Can this land?

CITGM looks OK (but this needs a rebase....)

Previously finished(stream, cb) would not invoke the callback for streams that have already finished, ended or errored before being passed to finished(stream, cb).

ronag · 2020-02-05T21:13:43Z

rebased

nodejs-github-bot · 2020-02-07T11:46:12Z

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

nodejs-github-bot · 2020-02-08T15:13:54Z

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

nodejs-github-bot · 2020-02-08T18:01:58Z

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

Previously finished(stream, cb) would not invoke the callback for streams that have already finished, ended or errored before being passed to finished(stream, cb). PR-URL: #31509 Refs: #31508 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

ronag · 2020-02-08T22:36:41Z

Landed in d016b9d

swarthy · 2020-02-20T06:47:39Z


  let writableFinished = stream.writableFinished ||
-    (stream._writableState && stream._writableState.finished);
+    (rState && rState.finished);


Looks like wState should be here instead of rState

nodejs#31509 introduced a slight typo. Fortunately this typo does not have big impact due to `isWritableFinished()`. Fixes: nodejs#31509 (comment)

#31509 introduced a slight typo. Fortunately this typo does not have big impact due to `isWritableFinished()`. Fixes: #31509 (comment) PR-URL: #31881 Fixes: #31509 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 25, 2020

ronag requested a review from mcollina January 25, 2020 16:33

ronag added semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem. labels Jan 25, 2020

ronag requested review from lpinca and mafintosh January 25, 2020 16:37

mcollina requested changes Jan 25, 2020

View reviewed changes

ronag requested a review from mcollina January 25, 2020 16:54

ronag force-pushed the eos-closed branch from 1fa0d4f to 9739e88 Compare January 25, 2020 16:56

ronag force-pushed the eos-closed branch from 7d918c2 to dc32141 Compare January 26, 2020 17:28

ronag mentioned this pull request Jan 26, 2020

stream: fix async iterator return when destroyed earlier #31508

Closed

3 tasks

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2020

Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2020

mcollina approved these changes Jan 27, 2020

View reviewed changes

ronag mentioned this pull request Jan 27, 2020

stream: fix finished writable/readable state #31527

Closed

4 tasks

ronag force-pushed the eos-closed branch from c29f548 to 066ca52 Compare January 29, 2020 07:59

This comment has been minimized.

Sign in to view

stream: finished should invoke callback for closed streams

f90f2a1

Previously finished(stream, cb) would not invoke the callback for streams that have already finished, ended or errored before being passed to finished(stream, cb).

ronag force-pushed the eos-closed branch from 066ca52 to f90f2a1 Compare February 5, 2020 21:13

fixup: bad merge

2203706

ronag closed this Feb 8, 2020

lpinca mentioned this pull request Feb 16, 2020

http: align with stream.Writable #31818

Closed

4 tasks

swarthy reviewed Feb 20, 2020

View reviewed changes

ronag mentioned this pull request Feb 20, 2020

stream: fix finished typo #31881

Closed

4 tasks

ronag added a commit to nxtedition/node that referenced this pull request Feb 20, 2020

stream: fix finished typo

242ccc2

nodejs#31509 introduced a slight typo. Fortunately this typo does not have big impact due to `isWritableFinished()`. Fixes: nodejs#31509 (comment)

ronag mentioned this pull request Feb 29, 2020

stream: make stream.destroy with callback API public #32021

Closed

4 tasks

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

ronag mentioned this pull request Apr 14, 2020

eos and pipeline will deadlock if streams are not in assumed state #28650

Closed

ronag mentioned this pull request Apr 25, 2020

doc: document major finished changes in v14 #33065

Closed

4 tasks

Uh oh!

Conversation

ronag commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jan 25, 2020

Uh oh!

lpinca commented Jan 25, 2020

Uh oh!

mcollina commented Jan 25, 2020

Uh oh!

ronag commented Jan 25, 2020

Uh oh!

ronag commented Jan 25, 2020

Uh oh!

lpinca commented Jan 25, 2020

Uh oh!

ronag commented Jan 25, 2020

Uh oh!

mcollina commented Jan 25, 2020

Uh oh!

ronag commented Jan 26, 2020

Uh oh!

nodejs-github-bot commented Jan 26, 2020

Uh oh!

ronag commented Jan 26, 2020

Uh oh!

nodejs-github-bot commented Jan 26, 2020

Uh oh!

nodejs-github-bot commented Jan 26, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

Trott commented Jan 27, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 31, 2020

Uh oh!

ronag commented Jan 31, 2020

Uh oh!

ronag commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Feb 1, 2020

Uh oh!

Trott commented Feb 5, 2020

Uh oh!

ronag commented Feb 5, 2020

Uh oh!

nodejs-github-bot commented Feb 7, 2020

Uh oh!

nodejs-github-bot commented Feb 8, 2020

Uh oh!

nodejs-github-bot commented Feb 8, 2020

Uh oh!

ronag commented Feb 8, 2020

Uh oh!

swarthy Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Jan 25, 2020 •

edited

Loading

ronag commented Jan 29, 2020 •

edited

Loading

ronag commented Jan 31, 2020 •

edited

Loading