stream: add catch handler for async _construct by jasnell · Pull Request #34416 · nodejs/node

jasnell · 2020-07-17T22:53:18Z

Add a catch handler for async _construct() for Streams.

class Foo extends stream.Duplex {
  async _construct(cb) {
    throw new Error('boom')
  }
}

const foo = new Foo()
foo.on('error', (error) => console.log(error.message))  // Prints 'boom'

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Add a catch handler for `async _construct()` for Streams.

nodejs-github-bot · 2020-07-17T22:54:24Z

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

mcollina

lgtm

ronag · 2020-07-18T05:23:24Z

If we do this for construct I think we should do it for _final, _flush and _destroy as well in the same go. I'm not sure about _read and _write due to performance concern.

jasnell · 2020-07-20T14:38:05Z

Pulling the author ready label back off so I can expand this as suggested by @ronag

jasnell · 2020-07-20T16:38:53Z

Ok, @ronag, @mcollina, @addaleax, and @antsmartian ... please take another look

As suggested by @ronag, this implements async function support for _construct(), _destroy(), _final(), _flush(), and _transform(). Not only does it handle the error cases when a rejected promise is returned, it allows using the resolved promise instead of callback (callback takes precedence if it is called).

As @ronag suspected, the performance hit of implementing this for _read() and _write() is way too significant (performance of _write() is cut in half) so while I've implemented it, I have not included it here.

addaleax · 2020-07-20T16:52:34Z

This still LGTM, but I kind of wish we could reduce code duplication a bit here…

jasnell · 2020-07-20T17:22:28Z

but I kind of wish we could reduce code duplication a bit here…

Yeah, I plan to take a second pass through with that in mind. I did it this way to minimize impact on any existing code. There are some timing differences in the Promise handler versions (deferrals to nextTick) that do not always work in the existing cases. Because of that, I wanted to focus first on making it work, then on reducing duplication later.

ronag · 2020-07-20T18:47:09Z

I like this... but I’m kind of unsure if we actually need this... especially if it’s non-trivial

jasnell · 2020-07-20T18:56:59Z

Need, possibly not, no more than we need Promise support in any of our APIs. It really comes down to modernizing the API, and the QUIC impl will immediately make use of the Promisified _construct. I'd say this falls into the same category as captureRejections in EventEmitter.

addaleax · 2020-07-20T19:03:01Z

I, for one, would really love to not ever need to write callback-based streams code again :)

nodejs-github-bot · 2020-07-20T19:38:39Z

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

ronag · 2020-07-20T19:58:13Z

I'm not sure I understand the need for the code duplication? Shouldn't this be a trivial case of:

function fn () {
  // ...
}
const thenable = this._construct(fn)
if (typeof thenable?.then === 'function') {
  thenable.then((val) => fn(null, val), (err) => fn(err))
}

Where fn is the existing implementation.

jasnell · 2020-07-20T20:08:46Z

For some cases, yes, in others, no, not without modifying the existing callbacks, which I don't want to do in this PR. I plan to eliminate / reduce the duplication by modifying the existing callbacks separately, once I can verify that there's no backwards compat issues

mcollina

I'll need some time to review this.

Can we check benchmarks to see if this this regress stuff? I don't expect it to, but this is complex code.

jasnell · 2020-07-21T18:11:40Z

Taking author ready back off so this does not land before @mcollina has a chance to do a more thorough review

jasnell · 2020-07-21T18:13:13Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/649/

12:59:28  streams/creation.js kind='duplex' n=50000000                                                             -0.77 %       ±3.28% ±4.36% ±5.68%
12:59:28  streams/creation.js kind='readable' n=50000000                                                            1.16 %       ±2.56% ±3.41% ±4.44%
12:59:28  streams/creation.js kind='transform' n=50000000                                                           1.03 %       ±3.33% ±4.44% ±5.78%
12:59:28  streams/creation.js kind='writable' n=50000000                                                            0.94 %       ±3.48% ±4.63% ±6.03%
12:59:28  streams/pipe.js n=5000000                                                                                -4.37 %       ±4.53% ±6.08% ±8.03%
12:59:28  streams/pipe-object-mode.js n=5000000                                                                    -1.05 %       ±4.43% ±5.91% ±7.70%
12:59:28  streams/readable-async-iterator.js sync='no' n=100000                                              *      3.26 %       ±2.54% ±3.38% ±4.40%
12:59:28  streams/readable-async-iterator.js sync='yes' n=100000                                                    1.26 %       ±3.86% ±5.13% ±6.68%
12:59:28  streams/readable-bigread.js n=1000                                                                        0.74 %       ±2.81% ±3.74% ±4.87%
12:59:28  streams/readable-bigunevenread.js n=1000                                                           *      5.82 %       ±4.42% ±5.90% ±7.72%
12:59:28  streams/readable-boundaryread.js type='buffer' n=2000                                              *     -2.49 %       ±2.24% ±2.98% ±3.88%
12:59:28  streams/readable-boundaryread.js type='string' n=2000                                                     0.29 %       ±1.82% ±2.42% ±3.14%
12:59:28  streams/readable-readall.js n=5000                                                                        1.67 %       ±3.12% ±4.15% ±5.41%
12:59:28  streams/readable-unevenread.js n=1000                                                                     1.85 %       ±2.67% ±3.56% ±4.63%
12:59:28  streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='no' n=2000000                     1.17 %       ±2.57% ±3.43% ±4.48%
12:59:28  streams/writable-manywrites.js len=1024 callback='no' writev='no' sync='yes' n=2000000                   -1.46 %       ±3.52% ±4.68% ±6.09%
12:59:28  streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='no' n=2000000                   -0.00 %       ±1.93% ±2.57% ±3.35%
12:59:28  streams/writable-manywrites.js len=1024 callback='no' writev='yes' sync='yes' n=2000000                   1.40 %       ±3.58% ±4.77% ±6.21%
12:59:28  streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='no' n=2000000                    1.40 %       ±2.21% ±2.95% ±3.88%
12:59:28  streams/writable-manywrites.js len=1024 callback='yes' writev='no' sync='yes' n=2000000                   1.75 %       ±3.52% ±4.69% ±6.10%
12:59:28  streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='no' n=2000000                   0.77 %       ±2.13% ±2.84% ±3.70%
12:59:28  streams/writable-manywrites.js len=1024 callback='yes' writev='yes' sync='yes' n=2000000                 -2.59 %       ±3.58% ±4.77% ±6.22%
12:59:28  streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='no' n=2000000                   -2.48 %       ±2.64% ±3.53% ±4.62%
12:59:28  streams/writable-manywrites.js len=32768 callback='no' writev='no' sync='yes' n=2000000                   0.46 %       ±4.02% ±5.35% ±6.97%
12:59:28  streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='no' n=2000000                  -0.13 %       ±2.08% ±2.77% ±3.61%
12:59:28  streams/writable-manywrites.js len=32768 callback='no' writev='yes' sync='yes' n=2000000                  0.91 %       ±3.73% ±4.96% ±6.47%
12:59:28  streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='no' n=2000000                  -1.55 %       ±2.46% ±3.29% ±4.31%
12:59:28  streams/writable-manywrites.js len=32768 callback='yes' writev='no' sync='yes' n=2000000                  0.95 %       ±4.16% ±5.54% ±7.21%
12:59:28  streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='no' n=2000000                  1.07 %       ±2.49% ±3.32% ±4.33%
12:59:28  streams/writable-manywrites.js len=32768 callback='yes' writev='yes' sync='yes' n=2000000                 0.19 %       ±3.59% ±4.77% ±6.22%

@mcollina... no performance change with these modifications

jasnell · 2020-07-31T19:27:28Z

Ping @mcollina

mcollina

lgtm, good work!

nodejs-github-bot · 2020-08-03T15:24:12Z

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

nodejs-github-bot · 2020-08-04T00:15:19Z

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

PR-URL: #34416 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>

jasnell · 2020-08-04T00:55:12Z

Landed in 744a284

addaleax · 2020-08-08T20:31:35Z

@jasnell This would need a manual backport for v14.x, there’s a bunch of conflicts

stream: add catch handler for async _construct

f74ff47

Add a catch handler for `async _construct()` for Streams.

jasnell added stream Issues and PRs related to the stream subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Jul 17, 2020

jasnell requested a review from ronag July 17, 2020 22:53

jasnell commented Jul 17, 2020

View reviewed changes

Comment thread lib/internal/streams/destroy.js

jasnell commented Jul 17, 2020

View reviewed changes

Comment thread test/parallel/test-stream-construct-async-error.js Outdated

jasnell requested a review from mcollina July 17, 2020 22:57

addaleax approved these changes Jul 17, 2020

View reviewed changes

mcollina approved these changes Jul 17, 2020

View reviewed changes

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

antsmartian approved these changes Jul 18, 2020

View reviewed changes

ronag reviewed Jul 18, 2020

View reviewed changes

Comment thread lib/internal/streams/destroy.js Outdated

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

stream: support async for stream impl functions

d1fd1fb

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

mcollina reviewed Jul 20, 2020

View reviewed changes

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

mcollina approved these changes Aug 1, 2020

View reviewed changes

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

This was referenced Aug 4, 2020

CI Relability 2020-08-04 JaneaSystems/nodejs-reliability#58

Open

CI Reliability 2020-08-04 nodejs/reliability#33

Open

jasnell closed this Aug 4, 2020

addaleax added the backport-requested-v14.x label Aug 8, 2020

This was referenced Aug 9, 2020

CI Relability 2020-08-09 JaneaSystems/nodejs-reliability#63

Open

CI Reliability 2020-08-09 nodejs/reliability#38

Open

CI Relability 2020-08-10 JaneaSystems/nodejs-reliability#64

Open

CI Reliability 2020-08-10 nodejs/reliability#39

Open

ronag mentioned this pull request Aug 11, 2020

stream: remove destroy thenable duplication #34723

Closed

omniadonum mentioned this pull request Jan 30, 2022

Readable._construct behaviour contradicts docs #39992

Open

Uh oh!

Conversation

jasnell commented Jul 17, 2020

Checklist

Uh oh!

nodejs-github-bot commented Jul 17, 2020

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

jasnell commented Jul 20, 2020

Uh oh!

jasnell commented Jul 20, 2020

Uh oh!

addaleax commented Jul 20, 2020

Uh oh!

jasnell commented Jul 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Jul 20, 2020

Uh oh!

addaleax commented Jul 20, 2020

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

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

Uh oh!

Uh oh!

jasnell commented Jul 20, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 21, 2020

Uh oh!

jasnell commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Jul 31, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 3, 2020

Uh oh!

nodejs-github-bot commented Aug 4, 2020

Uh oh!

jasnell commented Aug 4, 2020

Uh oh!

addaleax commented Aug 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ronag commented Jul 18, 2020 •

edited

Loading

ronag commented Jul 20, 2020 •

edited

Loading

ronag commented Jul 20, 2020 •

edited

Loading

jasnell commented Jul 21, 2020 •

edited

Loading