Skip to content

http: improve performance by removing async_hooks - #57746

Closed
JonasBa wants to merge 0 commit into
nodejs:mainfrom
JonasBa:main
Closed

http: improve performance by removing async_hooks#57746
JonasBa wants to merge 0 commit into
nodejs:mainfrom
JonasBa:main

Conversation

@JonasBa

@JonasBaJonasBa commented Apr 4, 2025

Copy link
Copy Markdown
Contributor

Pending benchmarks, but this should improve http request performance.

Github search results for HTTPIncomingMessage and HTTPClientRequest

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. needs-ci PRs that need a full CI run. labels Apr 4, 2025
@anonrig
anonrig requested a review from jasnellApril 4, 2025 15:25
@mcollina

mcollina commented Apr 4, 2025

Copy link
Copy Markdown
Member

Pending positive benchmarks, I’m in favor of this change as the http parser does not have inherent asynchronous behavior and it’s all sync.

@jasnelljasnell added the needs-benchmark-ci PR that need a benchmark CI run. label Apr 4, 2025
Comment threadsrc/node_http_parser.cc Outdated
Comment threadsrc/node_http_parser.cc Outdated
Comment threadsrc/node_http_parser.cc Outdated
@JonasBa
JonasBaforce-pushed the main branch 2 times, most recently from df87762 to a7399d4CompareApril 12, 2025 00:10
Comment threadsrc/node_http_parser.cc Outdated
Comment threadsrc/node_http_parser.cc Outdated
@FlarnaFlarna added the async_hooks Issues and PRs related to the async hooks subsystem. label Apr 17, 2025
@Flarna

Copy link
Copy Markdown
Member

HTTP parser instances are reused between HTTP requests (there is a pool of them). in contrast to parser HTTP requests are async and each HTTP request holds a parser.

There are some tests deleted in this PR which verify that each HTTP request is seen as an unique async resource.

AsyncLocalStore is/was built on top of async hooks (get current resource), as a result test coverage for ALS is quite limited because of this..

Therefore I fear that this is quite breaking for ALS users. I recommend to add tests similar to the deleted ones but using AsyncLocalStore API to ensure nothing is broken because of this removal.

@FlarnaFlarna added async_local_storage AsyncLocalStorage http Issues or PRs related to the http subsystem. labels Apr 17, 2025
Comment threadsrc/node_http_parser.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this. We just shouldn't throw if the previous js callback throws, but we shouldn't stop the flow when an error is thrown on the user handler.

@anonrig
anonrig marked this pull request as ready for review April 19, 2025 16:38
@codecov

codecovBot commented Apr 19, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 61.29032% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (1123585) to head (ee036b5).
Report is 169 commits behind head on main.

Files with missing linesPatch %Lines
src/node_http_parser.cc60.00%4 Missing and 8 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #57746 +/- ##
==========================================
+ Coverage 90.22% 90.24% +0.01% 
==========================================
Files 630 630 Lines 185055 186045 +990 Branches 36216 36455 +239 ==========================================
+ Hits 166975 167905 +930 + Misses 11042 10995 -47 - Partials 7038 7145 +107 
Files with missing linesCoverage Δ
lib/_http_client.js97.97% <ø> (-0.02%)⬇️
lib/_http_common.js100.00% <ø> (ø)
lib/_http_server.js96.98% <100.00%> (-0.01%)⬇️
src/node_http_parser.cc82.59% <60.00%> (-0.44%)⬇️

... and 120 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment threadlib/_http_server.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTPServerAsyncResource class can now be removed I believe.

@JonasBa

Copy link
Copy Markdown
ContributorAuthor

Sorry everyone, looks like the lack of sleep as a new parent caused me to not even notice that I pushed this from my main fork branch. I'll close this PR and reopen it on a proper branch, with a link to the discussions here.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

async_hooksIssues and PRs related to the async hooks subsystem.async_local_storageAsyncLocalStoragec++Issues and PRs that require attention from people who are familiar with C++.http_parserIssues and PRs related to the HTTP Parser dependency or the http_parser binding.httpIssues or PRs related to the http subsystem.needs-benchmark-ciPR that need a benchmark CI run.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@JonasBa@nodejs-github-bot@mcollina@Flarna@jasnell@anonrig