stream: add catch handler for async _construct

[jasnell]

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

classFooextendsstream.Duplex{async_construct(cb){thrownewError('boom')}}constfoo=newFoo()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

[nodejs-github-bot]

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

[mcollina]

lgtm

[ronag]

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]

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

[jasnell]

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]

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

[jasnell]

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]

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

[jasnell]

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]

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

[nodejs-github-bot]

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

[ronag]

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

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

Where fn is the existing implementation.

[jasnell]

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]

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

[jasnell]

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]

Ping @mcollina

[mcollina]

lgtm, good work!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[jasnell]

Landed in 744a284

[addaleax]

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