stream: add catch handler for async _construct #34416

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/

lib/internal/streams/destroy.js

test/parallel/test-stream-construct-async-error.js

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.

lib/internal/streams/destroy.js

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 semver-minor labels Jul 17, 2020

jasnell requested a review from ronag Jul 17, 2020

jasnell reviewed Jul 17, 2020

View changes

lib/internal/streams/destroy.js Show resolved Hide resolved

jasnell reviewed Jul 17, 2020

View changes

test/parallel/test-stream-construct-async-error.js Outdated Show resolved Hide resolved

jasnell requested a review from mcollina Jul 17, 2020

addaleax approved these changes Jul 17, 2020

View changes

mcollina approved these changes Jul 17, 2020

View changes

jasnell added the author ready label Jul 17, 2020

antsmartian approved these changes Jul 18, 2020

View changes

ronag reviewed Jul 18, 2020

View changes

lib/internal/streams/destroy.js Outdated Show resolved Hide resolved

jasnell removed the author ready label Jul 20, 2020

stream: support async for stream impl functions

d1fd1fb

jasnell added the author ready label Jul 20, 2020

mcollina reviewed Jul 20, 2020

View changes

jasnell removed the author ready label Jul 21, 2020

mcollina approved these changes Aug 1, 2020

View changes

jasnell added the author ready label Aug 3, 2020

github-actions bot mentioned this pull request Aug 4, 2020

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

Open

12 tasks

github-actions bot mentioned this pull request Aug 4, 2020

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

Open

13 tasks

jasnell added a commit that referenced this issue Aug 4, 2020

stream: support async for stream impl functions

744a284

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 closed this Aug 4, 2020

github-actions bot mentioned this pull request Aug 5, 2020

CI Relability 2020-08-05 JaneaSystems/nodejs-reliability#59

Open

12 tasks

github-actions bot mentioned this pull request Aug 5, 2020

CI Reliability 2020-08-05 nodejs/reliability#34

Open

12 tasks

github-actions bot mentioned this pull request Aug 6, 2020

CI Relability 2020-08-06 JaneaSystems/nodejs-reliability#60

Open

13 tasks

github-actions bot mentioned this pull request Aug 6, 2020

CI Reliability 2020-08-06 nodejs/reliability#35

Open

13 tasks

github-actions bot mentioned this pull request Aug 7, 2020

CI Relability 2020-08-07 JaneaSystems/nodejs-reliability#61

Open

15 tasks

github-actions bot mentioned this pull request Aug 7, 2020

CI Reliability 2020-08-07 nodejs/reliability#36

Open

15 tasks

github-actions bot mentioned this pull request Aug 8, 2020

CI Relability 2020-08-08 JaneaSystems/nodejs-reliability#62

Open

12 tasks

github-actions bot mentioned this pull request Aug 8, 2020

CI Reliability 2020-08-08 nodejs/reliability#37

Open

12 tasks

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

github-actions bot mentioned this pull request Aug 9, 2020

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

Open

12 tasks

github-actions bot mentioned this pull request Aug 9, 2020

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

Open

12 tasks

github-actions bot mentioned this pull request Aug 10, 2020

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

Open

12 tasks

github-actions bot mentioned this pull request Aug 10, 2020

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

Open

12 tasks

ronag mentioned this pull request Aug 11, 2020

stream: remove destroy thenable duplication #34723

Open

zamnuts mentioned this pull request Jan 30, 2022

Readable._construct behaviour contradicts docs #39992

Open

nodejs / node Public

stream: add catch handler for async _construct #34416

stream: add catch handler for async _construct #34416

jasnell commented Jul 17, 2020

nodejs-github-bot commented Jul 17, 2020

mcollina left a comment

ronag commented Jul 18, 2020 •

edited

jasnell commented Jul 20, 2020

jasnell commented Jul 20, 2020

addaleax commented Jul 20, 2020

jasnell commented Jul 20, 2020

ronag commented Jul 20, 2020 •

edited

jasnell commented Jul 20, 2020

addaleax commented Jul 20, 2020

nodejs-github-bot commented Jul 20, 2020

ronag commented Jul 20, 2020 •

edited

jasnell commented Jul 20, 2020

mcollina left a comment

jasnell commented Jul 21, 2020

jasnell commented Jul 21, 2020 •

edited

jasnell commented Jul 31, 2020

mcollina left a comment

nodejs-github-bot commented Aug 3, 2020

nodejs-github-bot commented Aug 4, 2020

jasnell commented Aug 4, 2020

addaleax commented Aug 8, 2020

nodejs / node Public

stream: add catch handler for async _construct #34416

stream: add catch handler for async _construct #34416

Conversation

jasnell commented Jul 17, 2020

Checklist

nodejs-github-bot commented Jul 17, 2020

mcollina left a comment

ronag commented Jul 18, 2020 • edited

jasnell commented Jul 20, 2020

jasnell commented Jul 20, 2020

addaleax commented Jul 20, 2020

jasnell commented Jul 20, 2020

ronag commented Jul 20, 2020 • edited

jasnell commented Jul 20, 2020

addaleax commented Jul 20, 2020

nodejs-github-bot commented Jul 20, 2020

ronag commented Jul 20, 2020 • edited

jasnell commented Jul 20, 2020

mcollina left a comment

jasnell commented Jul 21, 2020

jasnell commented Jul 21, 2020 • edited

jasnell commented Jul 31, 2020

mcollina left a comment

nodejs-github-bot commented Aug 3, 2020

nodejs-github-bot commented Aug 4, 2020

jasnell commented Aug 4, 2020

addaleax commented Aug 8, 2020

ronag commented Jul 18, 2020 •

edited

ronag commented Jul 20, 2020 •

edited

ronag commented Jul 20, 2020 •

edited

jasnell commented Jul 21, 2020 •

edited