stream: allow using `.push()`/`.unshift()` during `once('data')` by addaleax · Pull Request #34957 · nodejs/node

addaleax · 2020-08-28T15:15:16Z

Previously, the .push() or .unshift() call would just have jumped
straight to emitting a 'data' event, even if there were no listeners,
effectively just silently dropping the chunk.

Checklist

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

Previously, the `.push()` or `.unshift()` call would just have jumped straight to emitting a `'data'` event, even if there were no listeners, effectively just silently dropping the chunk.

nodejs-github-bot · 2020-08-28T15:21:01Z

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

benjamingr

Good catch!

nodejs-github-bot · 2020-08-28T18:33:59Z

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

nodejs-github-bot · 2020-08-28T20:56:02Z

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

addaleax · 2020-08-28T20:57:53Z

Benchmark CI as requested by @mcollina in #34958: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/662/

I don’t know what impact the result would have on the PR here, though. Unless this turns out to be a really bad performance regression, I’d say that correctness comes first.

lundibundi · 2020-08-29T09:51:03Z

Based on benchmarks the impact is negligible

01:28:09 confidence improvement accuracy (*) (**) (***)
01:28:09 streams/creation.js kind='duplex' n=50000000 -1.51 % ±2.87% ±3.82% ±4.97%
01:28:09 streams/creation.js kind='readable' n=50000000 -1.32 % ±2.46% ±3.27% ±4.26%
01:28:09 streams/creation.js kind='transform' n=50000000 1.09 % ±3.36% ±4.47% ±5.81%
01:28:09 streams/creation.js kind='writable' n=50000000 -0.80 % ±3.32% ±4.42% ±5.75%
01:28:09 streams/pipe.js n=5000000 1.03 % ±2.58% ±3.44% ±4.51%
01:28:09 streams/pipe-object-mode.js n=5000000 -1.82 % ±3.49% ±4.67% ±6.15%
01:28:09 streams/readable-async-iterator.js sync='no' n=100000 0.25 % ±2.58% ±3.43% ±4.47%
01:28:09 streams/readable-async-iterator.js sync='yes' n=100000 0.01 % ±4.45% ±5.93% ±7.71%
01:28:09 streams/readable-bigread.js n=1000 -2.38 % ±2.78% ±3.70% ±4.82%
01:28:09 streams/readable-bigunevenread.js n=1000 0.80 % ±5.46% ±7.27% ±9.47%
01:28:09 streams/readable-boundaryread.js type='buffer' n=2000 2.44 % ±3.76% ±5.03% ±6.60%
01:28:09 streams/readable-boundaryread.js type='string' n=2000 -0.21 % ±2.22% ±2.96% ±3.86%
01:28:09 streams/readable-readall.js n=5000 2.98 % ±3.28% ±4.36% ±5.68%
01:28:09 streams/readable-unevenread.js n=1000 -0.53 % ±1.43% ±1.91% ±2.48%
01:28:09 streams/writable-manywrites.js len=1024callback='no' writev='no' sync='no' n=2000000 -0.39 % ±1.32% ±1.76% ±2.29%
01:28:09 streams/writable-manywrites.js len=1024callback='no' writev='no' sync='yes' n=2000000 3.61 % ±4.43% ±5.89% ±7.67%
01:28:09 streams/writable-manywrites.js len=1024callback='no' writev='yes' sync='no' n=2000000 0.34 % ±2.34% ±3.11% ±4.05%
01:28:09 streams/writable-manywrites.js len=1024callback='no' writev='yes' sync='yes' n=2000000 0.73 % ±3.60% ±4.79% ±6.24%
01:28:09 streams/writable-manywrites.js len=1024callback='yes' writev='no' sync='no' n=2000000 -0.38 % ±1.37% ±1.82% ±2.38%
01:28:09 streams/writable-manywrites.js len=1024callback='yes' writev='no' sync='yes' n=2000000 -1.99 % ±3.29% ±4.37% ±5.69%
01:28:09 streams/writable-manywrites.js len=1024callback='yes' writev='yes' sync='no' n=2000000 1.05 % ±2.07% ±2.76% ±3.59%
01:28:09 streams/writable-manywrites.js len=1024callback='yes' writev='yes' sync='yes' n=2000000 -0.82 % ±2.90% ±3.87% ±5.03%
01:28:09 streams/writable-manywrites.js len=32768callback='no' writev='no' sync='no' n=2000000 0.30 % ±1.48% ±1.97% ±2.56%
01:28:09 streams/writable-manywrites.js len=32768callback='no' writev='no' sync='yes' n=2000000 -2.36 % ±3.70% ±4.93% ±6.42%
01:28:09 streams/writable-manywrites.js len=32768callback='no' writev='yes' sync='no' n=2000000 -1.18 % ±2.66% ±3.56% ±4.66%
01:28:09 streams/writable-manywrites.js len=32768callback='no' writev='yes' sync='yes' n=2000000 -1.09 % ±3.20% ±4.26% ±5.54%
01:28:09 streams/writable-manywrites.js len=32768callback='yes' writev='no' sync='no' n=2000000 -1.25 % ±3.54% ±4.73% ±6.20%
01:28:09 streams/writable-manywrites.js len=32768callback='yes' writev='no' sync='yes' n=2000000 -1.66 % ±3.22% ±4.29% ±5.59%
01:28:09 streams/writable-manywrites.js len=32768callback='yes' writev='yes' sync='no' n=2000000 * 3.31 % ±3.05% ±4.08% ±5.34%
01:28:09 streams/writable-manywrites.js len=32768callback='yes' writev='yes' sync='yes' n=2000000 1.51 % ±4.08% ±5.43% ±7.09%
01:28:09 01:28:09 Be aware that when doing many comparisons the risk of a false-positive
01:28:09 result increases. In this case there are 30 comparisons, you can thus
01:28:09 expect the following amount of false-positive results:
01:28:09 1.50 false positives, when considering a 5% risk acceptance (*, **, ***),
01:28:09 0.30 false positives, when considering a 1% risk acceptance (**, ***),
01:28:09 0.03 false positives, when considering a 0.1% risk acceptance (***)

nodejs-github-bot · 2020-08-29T09:53:06Z

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

ronag

RSLGTM

Previously, the `.push()` or `.unshift()` call would just have jumped straight to emitting a `'data'` event, even if there were no listeners, effectively just silently dropping the chunk. PR-URL: #34957 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

Trott · 2020-08-30T15:43:19Z

Landed in c1ef122

Previously, the `.push()` or `.unshift()` call would just have jumped straight to emitting a `'data'` event, even if there were no listeners, effectively just silently dropping the chunk. PR-URL: #34957 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com>

stream: allow using .push()/.unshift() during once('data')
d21c409
Previously, the `.push()` or `.unshift()` call would just have jumped straight to emitting a `'data'` event, even if there were no listeners, effectively just silently dropping the chunk.

addaleax added stream Issues and PRs related to the stream subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 28, 2020

addaleax mentioned this pull request Aug 28, 2020
http2,tls: account for buffered data before creating socket wraps #34958
Closed
3 tasks

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2020

benjamingr approved these changes Aug 28, 2020
View reviewed changes

lpinca approved these changes Aug 28, 2020
View reviewed changes

cjihrig approved these changes Aug 28, 2020
View reviewed changes

Trott requested a review from ronag August 28, 2020 18:54

Trott approved these changes Aug 28, 2020
View reviewed changes

lundibundi approved these changes Aug 29, 2020
View reviewed changes

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

ronag approved these changes Aug 29, 2020
View reviewed changes

rickyes approved these changes Aug 30, 2020
View reviewed changes

Trott closed this Aug 30, 2020

addaleax deleted the stream-add-chunk-during-data branch August 30, 2020 18:52

richardlau mentioned this pull request Sep 2, 2020
v14.10.0 proposal #35023
Merged
4 tasks

codebytere mentioned this pull request Sep 28, 2020
v12.19.0 proposal #35401
Merged

Navigation Menu

Uh oh!

Uh oh!

stream: allow using `.push()`/`.unshift()` during `once('data')` - #34957

stream: allow using `.push()`/`.unshift()` during `once('data')`#34957
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:stream-add-chunk-during-data

addaleax commented Aug 28, 2020

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

benjamingr left a comment

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

addaleax commented Aug 28, 2020

Uh oh!

lundibundi commented Aug 29, 2020

Uh oh!

nodejs-github-bot commented Aug 29, 2020

Uh oh!

ronag left a comment

Uh oh!

Trott commented Aug 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Uh oh!

Conversation

addaleax commented Aug 28, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

nodejs-github-bot commented Aug 28, 2020

Uh oh!

addaleax commented Aug 28, 2020

Uh oh!

lundibundi commented Aug 29, 2020

Uh oh!

nodejs-github-bot commented Aug 29, 2020

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants