-
Notifications
You must be signed in to change notification settings - Fork 28.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stream: add catch handler for async _construct #34416
Conversation
Add a catch handler for `async _construct()` for Streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
If we do this for construct I think we should do it for |
Pulling the |
Ok, @ronag, @mcollina, @addaleax, and @antsmartian ... please take another look As suggested by @ronag, this implements async function support for As @ronag suspected, the performance hit of implementing this for |
This still LGTM, 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. |
I like this... but I’m kind of unsure if we actually need this... especially if it’s non-trivial |
Need, possibly not, no more than we need |
I, for one, would really love to not ever need to write callback-based streams code again :) |
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 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Taking |
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/649/
@mcollina... no performance change with these modifications |
Ping @mcollina |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, good work!
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>
Landed in 744a284 |
@jasnell This would need a manual backport for v14.x, there’s a bunch of conflicts |
Add a catch handler for
async _construct()
for Streams.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes