Skip to content

http: improve performance by removing async_hooks #57938

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JonasBa
Copy link
Contributor

@JonasBa JonasBa commented Apr 20, 2025

Reopening this from #57746.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net
  • @nodejs/performance

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 20, 2025
@JonasBa JonasBa force-pushed the jb/node/http-parser branch from ee036b5 to 7dfbf7e Compare April 20, 2025 00:36
@Flarna Flarna added async_hooks Issues and PRs related to the async hooks subsystem. async_local_storage AsyncLocalStorage http Issues or PRs related to the http subsystem. labels Apr 22, 2025
@Flarna
Copy link
Member

Flarna commented Apr 23, 2025

I marked this as semver major.
Removing async hooks for HTTP and as a result likely breaking AsyncLocalStore for HTTP requests will have significant impact to at least OpenTelemetry and other APM tools.

While I agree that HTTP parser itself is not async HTTP requests are. As a result I'm fine to the change of http parser itself but the async hooks functionality itself should preserved on HTTP request level otherwise then.

@Flarna Flarna added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 23, 2025
@Qard
Copy link
Member

Qard commented Apr 24, 2025

@Flarna have you verified that it's still relevant with the AsyncContextFrame-backed AsyncLocalStorage? I have a feeling it might not matter anymore, but not 100% certain about that. 🤔

@Flarna
Copy link
Member

Flarna commented Apr 24, 2025

No, I haven't verified this. Basically this reverts fixes done a while ago (#27477 and #25094) to prepare HTTP for a better AsyncLocalStorage implementation.

Could be that AsyncContextFrame-backed AsyncLocalStorage solves this meanwhile, but it could be also that the move to AsyncContextFrame broke it already.
To my understanding AsyncContextFrame solution either follows promises/microtasks automatically inside v8, or it's told from outside. This happens for example in AsyncResource class on JS side or AsyncWrap class in C++. And exactly this AsyncWrap is removed here from HTTP parser.

In the end we don't know as there are no test (ref: #55712 - I thought I find some more time to add tests...).

@Flarna
Copy link
Member

Flarna commented Apr 25, 2025

Created #58017 to add at least one more test for ALS + HTTP.

At the moment this branch doesn't build so couldn't do any local testing. As it is in draft state this is fine and there is no need to hurry anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. async_local_storage AsyncLocalStorage c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants