http: remove async tracking from http parser

[anonrig]

Continues the work of @JonasBa on #57938

Improves the performance of http parser by 5% by removing the async_hooks on an synchronous http parser. I'm not sure why we had async_hooks in the first place. @mcollina was the first person who made me realize about this, so kudos to him for realizing this in unneeded.

[nodejs-github-bot]

Review requested:

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

[jasnell]

Is this actually correct for correct error propagation? @addaleax

[addaleax]

Doesn't seem like it, no.

I honestly don't quite get what the idea behind this PR is. Maybe @anonrig or @mcollina can expand on this a bit more, and ideally incorporate it into the commit message, so that it's accessible not just for us as reviewers but also for future context. But as it stands, the correct title for the PR should be http: remove async tracking, not http: improve http parser performance.

[anonrig]

Thank you for the reviews!

I've surrounded this line with v8::TryCatch and handled the error according (and reported it to llhttp). I'll add necessary tests before landing this. Let me know if this addresses your concerns @addaleax

[codecov]

Codecov Report

❌ Patch coverage is 59.09091% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.92%. Comparing base (255dd7b) to head (0e7a62e).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_http_parser.cc	59.09%	9 Missing and 9 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59621      +/-   ##
==========================================
- Coverage   89.93%   89.92%   -0.01%     
==========================================
  Files         667      667              
  Lines      196790   196759      -31     
  Branches    38423    38406      -17     
==========================================
- Hits       176982   176941      -41     
- Misses      12240    12263      +23     
+ Partials     7568     7555      -13     

Files with missing lines	Coverage Δ
lib/_http_client.js	97.31% <ø> (-0.03%)	⬇️
lib/_http_common.js	100.00% <ø> (ø)
lib/_http_server.js	96.93% <ø> (-0.02%)	⬇️
src/node_http_parser.cc	82.88% <59.09%> (-1.39%)	⬇️

... and 33 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.

[mcollina]

@jasnell echoing what @Qard has said, I've marked it as dont-land on both 20.x and 22.x. I don't think there are problems in landing this on 24.

[addaleax]

I'm not sure why we had async_hooks in the first place.

We have the AsyncWrap integration, and more generally 'callback handling' logic (CallbackScope or MakeCallback()) for the same reason we have it anywhere else in Node.js -- when we are entering JS from C++ (without a JS call stack underneath), we need to take care of things like handling uncaught exceptions and running the microtask queue, and, well, async context tracking.

Minimal regression test for the type of bug that would be introduced by this PR:

'use strict';
const { get, createServer } = require('node:http');
const { executionAsyncId } = require('node:async_hooks');
const assert = require('node:assert');

createServer((req, res) => {
  assert.notStrictEqual(executionAsyncId(), 0); // this test makes this fail
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end('Hello, World!\n');
}).listen(0, function() {
  get(`http://localhost:${this.address().port}/`, () => this.close());
});

[addaleax]

What happens to these exceptions?

[anonrig]

This particular line handles it:

// If there was an exception in one of the callbacks
    if (got_exception_)
      return scope.Escape(Local<Value>());

[addaleax]

Nope:

'use strict';
const { get, createServer } = require('node:http');
const { executionAsyncId } = require('node:async_hooks');
const assert = require('node:assert');

createServer((req, res) => {
  throw new Error('oops this exception is silently swallowed!');
}).listen(0, function() {
  get(`http://localhost:${this.address().port}/`, () => this.close());
});

[addaleax]

Doesn't seem like it, no.

I honestly don't quite get what the idea behind this PR is. Maybe @anonrig or @mcollina can expand on this a bit more, and ideally incorporate it into the commit message, so that it's accessible not just for us as reviewers but also for future context. But as it stands, the correct title for the PR should be http: remove async tracking, not http: improve http parser performance.

[addaleax]

Also, just going to echo what @Flarna said in the original PR:

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...).

I agree that the most meaningful start for making this sort of optimization is to ensure that we have the necessary test coverage before jumping to conclusions about what is and isn't feasible or acceptable.

[Flarna]

I'm not sure why we had async_hooks in the first place.

see corresponding comment in original PR:

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.

Quite a while ago I tried such an approach to use the http.IncomingMessage instance as async resource instead the http parser. Background that time was to avoid the need for an asnc resource reset on the http parser because it's reused.
But it turned out to be not that trivial and I stopped this approach. Maybe someone with more experience in node source has more luck.

[mcollina]

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.

This can be easily achieved using an AsyncResource, no?

[addaleax]

@mcollina Yeah, it's possible to do async tracking manually in JS, and arguably at least as good.

That's a problem with this PR, but it's not really the main problem (although it does show that having async tracking entangled with other C++-to-JS call logic can be a bit of an obstacle).

[Flarna]

This can be easily achieved using an AsyncResource, no?

It can be done for sure but I can't tell if it is easy. The HTTPParser gets data from some socket and emits events like onHeadersComplete or onBody whenever it detects this during parsing.

Currently HTTPParser takes care to switch async context from socket context to http request context. This can be for sure done in a separate AsyncResource by binding these callbacks.

I can't tell out of the box if moving this functionality from HttpParser to a dedicated AsyncResource improves the HTTP performance. While HTTPParser might be faster HTTP as a whole could be even slower in the end.

FWIW I would assume that parsing speed itself in HTTPParser is not effected at all by async hooks. Only init and the callbacks should be effected.
Would be interesting for which cases (small/big payload,...) we can get the 5% performance gain.

[addaleax]

Okay, to reiterate, this PR improves performance by breaking or removing:

• Async Hooks support
• Async Context Frame tracking
• Uncaught exception handling
• WeakRef handling
• Microtask scheduling/relative timing

You're breaking all of these at the same time, which I don't think is what you're intending.

I don't think this PR is one that you can realistically iterate on – if you seriously want to pursure this, you'll need to think about what exactly you are intending to achieve, what level of breakage you are okay with accepting, and make a plan for how to get there.

[addaleax]

Nope:

'use strict';
const { get, createServer } = require('node:http');
const { executionAsyncId } = require('node:async_hooks');
const assert = require('node:assert');

createServer((req, res) => {
  throw new Error('oops this exception is silently swallowed!');
}).listen(0, function() {
  get(`http://localhost:${this.address().port}/`, () => this.close());
});

[addaleax]

Also, I feel like there's a misconception here – quoting @mcollina from #57746:

[...] as the http parser does not have inherent asynchronous behavior and it’s all sync.

That statement is, at best, misleading. While the HTTP parser logic is synchronous, HTTP parser callbacks into JS are absolutely asynchronous in the same way that a socket data callback is (because that's essentially what it is!), and there's no JS stack or underneath those callbacks.

[mcollina]

That statement is, at best, misleading. While the HTTP parser logic is synchronous, HTTP parser callbacks into JS are absolutely asynchronous in the same way that a socket data callback is (because that's essentially what it is!), and there's no JS stack or underneath those callbacks.

We may have a different interpretation of what asynchronous means in this context. The chunks are in JS, which calls another JS function (implemented in C++), which in turn calls other JS functions, all of it without passing through the event loop. How is this misleading?

If we lose the JS stack information, it's a problem in this PR / our binding.
I've seen the stacks in Undici with our wasm binding - so this is possible.

BTW, I agree with you on everything you mentioned in #59621 (review), but I think it's possible to iterate on this PR and see where we land in term of breakage. The goal is to break nothing, minus some microtick timing and async_hooks resource naming.

[jasnell]

I think @addaleax is right in that I don't think it's necessarily possible to iterate on this PR. It's going to require taking a step back and deciding exactly what set of changes are needed here. What specifically is the goal?

Let's put the questions over whether it is or is not async aside and focus on the requirements for a change here. I'd recommend closing this PR (or moving it to draft), taking a step back, and trying again.

[mcollina]

What specifically is the goal?

The goal is improving the performance of the HTTP stack.

[jasnell]

The goal is improving the performance of the HTTP stack.

Ok, then definitely this PR needs to take a big step back and rethink the problem :-)

[addaleax]

The chunks are in JS, which calls another JS function (implemented in C++), which in turn calls other JS functions, all of it without passing through the event loop. How is this misleading?

@mcollina My apologies, I have to partially retract my statement here – on the client side, apparently we're not consuming sockets through C++ 🤯 I was looking at HTTP server instances before, and in those cases we are 100% directly consuming data from the socket (and potentially through TLS) to llhttp, all in C++ land.

This is honestly pretty shocking news to me, because there's absolutely NO good reason why it would be that way only for servers and not clients. It's also very good news – it means we can noticeably improve HTTP parser performance without breaking anything at all.

As an extreme example, this naïve 20-line diff:

Diff in the fold

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 63a7befc8ebb..cd224857c199 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -97,6 +97,8 @@ const onClientRequestStartChannel = dc.channel('http.client.request.start');
 const onClientRequestErrorChannel = dc.channel('http.client.request.error');
 const onClientResponseFinishChannel = dc.channel('http.client.response.finish');
 
+const kOnExecute = HTTPParser.kOnExecute | 0;
+
 function emitErrorEvent(request, error) {
   if (onClientRequestErrorChannel.hasSubscribers) {
     onClientRequestErrorChannel.publish({
@@ -612,6 +614,17 @@ function socketOnData(d) {
   assert(parser && parser.socket === socket);
 
   const ret = parser.execute(d);
+  onParserExecuteCommon(socket, ret, d);
+}
+
+function onParserExecute(socket, ret) {
+  onParserExecuteCommon(socket, ret, undefined);
+}
+
+function onParserExecuteCommon(socket, ret, d) {
+  const req = socket._httpMessage;
+  const parser = socket.parser;
+
   if (ret instanceof Error) {
     prepareError(ret, parser, d);
     debug('parse error', ret);
@@ -626,6 +639,7 @@ function socketOnData(d) {
     const bytesParsed = ret;
     const res = parser.incoming;
     req.res = res;
+    d ||= parser.getCurrentBuffer();
 
     socket.removeListener('data', socketOnData);
     socket.removeListener('end', socketOnEnd);
@@ -911,6 +925,14 @@ function tickOnSocket(req, socket) {
     listenSocketTimeout(req);
   }
   req.emit('socket', socket);
+
+  if (/* hacky, should match server */ socket._handle) {
+    parser._consumed = true;
+    socket._handle._consumed = true;
+    parser.unconsume();
+    parser.consume(socket._handle);
+  }
+  parser[kOnExecute] = onParserExecute.bind(socket, socket);
 }
 
 function emitRequestTimeout() {

also results in a 5% improvement:

$ ./node benchmark/compare.js --old ./node-main --new ./node --filter simple --set chunkedEnc=1 --set type=bytes --set len=102400 --runs 10 http > compare.csv && npx node-benchmark-compare ./compare.csv
[00:06:47|% 100| 1/1 files | 20/20 runs | 4/4 configs]: Done
                                                                                                             confidence improvement accuracy (*)   (**)  (***)
http/simple.js duration=5 chunkedEnc=1 c=50 chunks=1 len=102400 type='bytes' benchmarker='test-double-http'         ***      7.20 %       ±1.89% ±2.62% ±3.62%
http/simple.js duration=5 chunkedEnc=1 c=50 chunks=4 len=102400 type='bytes' benchmarker='test-double-http'         ***      4.41 %       ±1.96% ±2.68% ±3.65%
http/simple.js duration=5 chunkedEnc=1 c=500 chunks=1 len=102400 type='bytes' benchmarker='test-double-http'        ***      6.43 %       ±2.06% ±2.83% ±3.86%
http/simple.js duration=5 chunkedEnc=1 c=500 chunks=4 len=102400 type='bytes' benchmarker='test-double-http'        ***      4.74 %       ±1.98% ±2.71% ±3.70%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 4 comparisons, you can thus expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

while even passing the vast majority of HTTP tests. Now, obviously, this will require a bit of polishing and effort to make sure that it really isn't breaking real-world code, but it's a great start imo.

[Qard]

I think it would actually be a good idea for the server to be doing the same thing. For a long time we pushed toward doing more in C++ to minimize when we call into JS, but that also resulted in lots of specialized internal systems that might be harder to optimize or simply might not get the attention to optimize them much.

With fast-calls, we could probably get similar or possibly better performance since things would be more visible to the optimizer. That'd also enable untangling some overly interconnected systems and make it possible to make things like the http parser logic more modular.

I think it has to some extent been a problem that the assumption when pursuing performance in Node.js is often that rewriting in C++ is the most obvious path. More consideration needs to be made for the context in which code will run. For example, HTTP parsing could be WASM for both client and server, which could not only skip an additional native -> JS barrier and be JIT optimizable, but it could also allow writing all the HTTP logic directly on top of the public TCP socket APIs rather than having a ton of custom code. Node.js gets overly complicated in a lot of cases due to subtle differences between, for example, native and JS streams.

I really wish Node.js core was better at keeping systems consistent and not over-engineering in pursuit of micro-benchmark performance. Performance is good, but maintainable systems are better, and isolated components tend to optimize much better than entangled systems where it's hard to know what other things might be impacted by a given change. Case in point: this exact PR. HTTP Parser should never have been considered async in the first place as it only inherits asynchrony via its caller. It's a similar case to event emitters--they are not async unless the thing calling them is, which is why event emitters don't do context binding by default.

[ShogunPanda]

@Qard I agree with you. We can move HTTP parsing to WASM even right now via llhttp wasm (which is already what undici is doing).
If I ever get to finish milo, WASM will be the preferred option.

[addaleax]

@Qard I think that's a great long-term plan, and I pretty much agree with everything you said 🙂

That being said, with the current setup, I do think it is the best short-term improvement to align server and client by removing the indirection through JS streams. That's a pretty self-contained improvement, too, and has the potential be a good first step to reduce the overall complexity of our HTTP handling by reducing duplication between server and client logic, if done right.

[Qard]

Yes, it's definitely a lot of work to get to that ideal future. I do think it's reasonable to go the other way though of pulling HTTP parsing out from servers and doing that in JS with WASM. I know there has already been talk and experimentation in that realm, though to what degree of completion I don't know precisely.

[mcollina]

@addaleax on the server, we are passing through this:

node/lib/_http_server.js

Line 744 in a7fde8a

socket.on('data', state.onData);

. So the data is passing through JS, or maybe is there a bypass I'm missing?

[addaleax]

So the data is passing through JS, or maybe is there a bypass I'm missing?

Yes, there is, a few lines below that one:

node/lib/_http_server.js

Line 767 in a7fde8a

parser.consume(socket._handle);