perf: avoid static AsyncResource.bind - #173
Conversation
|
cc: @Phillip9587 |
|
with body-parser size mode | 3.0.2 req/s | master req/s | 302 p99 mstr p99 | delta winnersmall raw | 28277.03 | 22710.16 | 3.59ms 9.93ms | -19.7% 302 after: scenario | 3.0.2 | master | deltasmall/raw | 28375 | 28491 | +0.4% |
|
First of all, I really appreciate all the work you've been putting into the project. Your contributions have been a huge benefit to the ecosystem, and it's great to see how much effort and energy you're investing. That said, I'd like to ask one thing: please leave your PRs open a bit longer for reviewers like me. Your last few PRs were all merged within about an hour, which didn't leave any time for others to review them. I know you're eager to ship improvements to the whole ecosystem, and I appreciate that. But please leave maintainers and reviewers some time to evaluate the changes, discuss alternatives, and consider the broader implications before merging. In my opinion, that's an important part of a healthy review process and leads to better long-term decisions. |
|
But there isn't really anyone else here, and this project isn't part of Express.js, which is why I've been moving changes quickly. I'm also making sure everything is thoroughly verified before merging. If you'd like to get involved, you're more than welcome. I mainly wanted to let you know about the change because I believe something similar was done in on-finished. |
|
Thanks for the clarification. And just to be clear, I really appreciate the work you're doing here. My only request is to leave PRs open a bit longer before merging. I'd love to have some time to review changes and provide feedback. Even if it feels a bit slower, giving reviewers (who also have work and other commitments) a chance to take a look would be greatly appreciated. |
|
I'd also be more than happy to review and help with changes to |
|
Cool. Well, I'm planning to release this version on Saturday, so if you want to take a look, feel free. If you need more time, just let me know, and we can make any final tweaks before the release. |
|
@Phillip9587 , I'm going to add you as a committer. |
The purpose of wrapping the callback in an AsyncResource is to propagate the async execution context, so that AsyncLocalStorage (per-request data) and APM tools built on async_hooks (Datadog, New Relic, OpenTelemetry) see the correct context inside the callback. That behavior remains exactly the same: both implementations invoke done within the context captured when getRawBody was called. I verified this above (ctx.id = req-1 in both cases) and across all 119 tests.
They also both create exactly one AsyncResource per request, just as before, so the number of init/destroy events is unchanged. Any tooling that counts async resources will observe no difference.
The only observable difference is .asyncResource.
The static helper attaches an .asyncResource property to the bound function. this implementation does not. And that's actually a good thing: that property is deprecated. Node already emits DeprecationWarning: The asyncResource property on bound functions is deprecated (DEP0172).