http: optimize IncomingMessage._dump #59747

ronag · 2025-09-03T15:44:59Z

PRE:

┌───────────┬─────────┬─────────┬─────────┬─────────┬──────────┬──────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg      │ Stdev    │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼──────────┼─────────┤
│ Req/Sec   │ 88,319  │ 88,319  │ 95,679  │ 96,447  │ 93,885.1 │ 2,779.85 │ 88,303  │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼──────────┼─────────┤
│ Bytes/Sec │ 10.8 MB │ 10.8 MB │ 11.7 MB │ 11.8 MB │ 11.5 MB  │ 339 kB   │ 10.8 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴──────────┴──────────┴─────────┘

POST:

┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 93,823  │ 93,823  │ 100,287 │ 101,119 │ 99,680  │ 1,967.7 │ 93,817  │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 11.5 MB │ 11.5 MB │ 12.2 MB │ 12.3 MB │ 12.2 MB │ 238 kB  │ 11.4 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

nodejs-github-bot · 2025-09-03T15:45:05Z

Review requested:

@nodejs/http
@nodejs/net

mcollina

lgtm

github-actions · 2025-09-03T20:31:03Z

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - http: optimize IncomingMessage._dump
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/17445334596

lib/_http_server.js

doc/api/http.md

ronag · 2025-09-04T13:22:15Z

@RafaelGSS why did you close?

RafaelGSS · 2025-09-04T13:50:10Z

I was a mistake, sorry @ronag

lpinca · 2025-09-04T19:39:53Z

doc/api/http.md

@@ -3632,6 +3635,10 @@ changes:
  * `rejectNonStandardBodyWrites` {boolean} If set to `true`, an error is thrown
    when writing to an HTTP response which does not have a body.
    **Default:** `false`.
+  * `dumpGETBody` {boolean} If set to `true`, request body for `HEAD` and `GET`
+    requests will be immediatly dumped and the `end` and `close` events for
+    `IncomingMessage` will be emitted before the server emits `'request'`. This


I understand that this is behind an option but I find it odd. It breaks the invariant where 'end' / 'close' are always emitted.

This is a valid point. I don't think it's anywhere documented that it must emit the life cycle events. Though it is indeed breaking. It is totally valid from the streams api since you can receive an already closed stream. It will work with stream.finished and stream.pipeline.

ronag · 2025-09-04T20:08:14Z

@mcollina @RafaelGSS benchmark CI is not meaningful, I added my own autocannon benchmark #59747 (comment)

const http = require('http');

http.createServer((req, res) => {
  res.end();
}).listen(9000);

npx autocannon http://localhost:9000

mcollina · 2025-09-04T20:21:09Z

Can we implement so that if there is someone watching for ‘end’ or ‘close’, it follows the usual path, while if they are not, we can avoid them?

Alternatively, we could have _dump actually emit them.

ronag · 2025-09-04T20:25:57Z

With fastify:

nxt-23.0$ npx autocannon http://localhost:9000/
Running 10s test @ http://localhost:9000/
10 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max    │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼────────┤
│ Latency │ 0 ms │ 0 ms │ 0 ms  │ 0 ms │ 0.03 ms │ 0.84 ms │ 137 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬──────────┬───────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg      │ Stdev     │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼───────────┼─────────┤
│ Req/Sec   │ 23,711  │ 23,711  │ 50,911  │ 79,487  │ 54,421.6 │ 22,087.53 │ 23,701  │
├───────────┼─────────┼─────────┼─────────┼─────────┼──────────┼───────────┼─────────┤
│ Bytes/Sec │ 4.32 MB │ 4.32 MB │ 9.27 MB │ 14.5 MB │ 9.9 MB   │ 4.02 MB   │ 4.31 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴──────────┴───────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

544k requests in 10.04s, 99 MB read
nxt-23.0$ npx autocannon http://localhost:9000/
Running 10s test @ http://localhost:9000/
10 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 0 ms │ 0 ms │ 0 ms  │ 0 ms │ 0.01 ms │ 0.27 ms │ 76 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬─────────┬─────────┬────────┬─────────┬─────────┬──────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%    │ 97.5%   │ Avg     │ Stdev    │ Min     │
├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼──────────┼─────────┤
│ Req/Sec   │ 68,287  │ 68,287  │ 82,175 │ 84,223  │ 81,056  │ 4,446.14 │ 68,272  │
├───────────┼─────────┼─────────┼────────┼─────────┼─────────┼──────────┼─────────┤
│ Bytes/Sec │ 12.4 MB │ 12.4 MB │ 15 MB  │ 15.3 MB │ 14.8 MB │ 809 kB   │ 12.4 MB │
└───────────┴─────────┴─────────┴────────┴─────────┴─────────┴──────────┴─────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

811k requests in 10.02s, 148 MB read

RafaelGSS · 2025-09-04T20:39:55Z

Benchmark result on my tests:

So, tuns out my computer was too fast and it was delivering the same amount of req/sec regardless of binary. In other words, I was being limited by wrk2 itself. Now I have configured proper values and the latency (which is the most important metric in terms of wrk2) has shown the following distribution:

 50.000%  731.00us
 75.000%    1.02ms
 90.000%    1.22ms
 99.000%    1.85ms
 99.900%    2.05ms
 99.990%    2.15ms
 99.999%    2.26ms
100.000%    2.49ms

node v24.5.0

 50.000%  693.00us
 75.000%    0.98ms
 90.000%    1.16ms
 99.000%    1.51ms
 99.900%    1.99ms
 99.990%    2.18ms
 99.999%    2.60ms
100.000%    2.94ms

And autocannon reported:

node v24.5.0

3164k requests in 30.04s, 576 MB read

versus

4007k requests in 30.04s, 729 MB read

lib/_http_server.js

Flarna · 2025-09-05T07:35:12Z

lib/_http_server.js

+    req._readableState.endEmitted = true;
+    req._readableState.destroyed = true;
+    req._readableState.closed = true;
+    req._readableState.closeEmitted = true;


Maybe move this stream internal handling into some function exported by the stream implementation?

_dumped is specific to http.

I refer to all the req._readableState.* assignments which are not http specific to my knowledge. The look more like stream internals.

Flarna · 2025-09-05T07:38:32Z

It seems the user facing option is gone now and this behavior is no opt in anymore. The repeated forced-pushes make this PR a bit hard to follow.

I think the PR headline/description should be improved a bit. Currently a reader might not expect that this optimization of an internal function is actually a breaking change.

ronag · 2025-09-05T08:47:01Z

Changed to draft.

Co-authored-by: Gürgün Dayıoğlu <[email protected]>

pimterry · 2025-09-05T08:49:42Z

It seems the user facing option is gone now and this behavior is no opt in anymore.

Indeed - that's definitely a problem imo, and this behaviour has to be optional (at least an option to disable it, but probably a opt-in option for a smoother transition).

There will definitely be plenty of cases in production where users do actually use GET bodies even though they shouldn't (we even have examples in our own tests, which have been fixed here) and there's real-world scenarios (like the ElasticSearch API) where supporting bodies with GET is required. Happy to discourage users from doing this, but we do have to support it for people who need it.

gurgunday · 2025-09-05T09:36:05Z

Just for context, even for Fastify we get a lot of people wanting to have a body in GET requests unfortunately

ronag · 2025-09-05T09:52:38Z

One option is to check the content-length header and only apply this optimization if it's not 0?

wesleytodd · 2025-09-05T14:31:44Z

For express I ran a quick run through our (work in progress) tooling on 22.19.0-bookworm and @ronag suggested to share here as well.

$ npm run load -- --test="@expressjs/perf-load-example" --overrides='{"express":"O4FDev/express#feat/dump-bodies-get-head"}'
$ npm run load -- --test="@expressjs/perf-load-example"
$ npm run compare  perf/load/example/results/result-1757081729427.json perf/load/example/results/result-1757081805041.json
> @expressjs/[email protected] compare
> expf compare perf/load/example/results/result-1757081729427.json perf/load/example/results/result-1757081805041.json
A Results: perf/load/example/results/result-1757081729427.json
==============================================================
┌─────────┬─────────┬─────────┬─────────┬─────────┬────────────┬───────────┬─────────┐
│ Stat    │ 2.5%    │ 50%     │ 97.5%   │ 99%     │ Avg        │ Stdev     │ Max     │
├─────────┼─────────┼─────────┼─────────┼─────────┼────────────┼───────────┼─────────┤
│ Latency │ 1082 ms │ 1119 ms │ 2807 ms │ 3710 ms │ 1358.19 ms │ 534.84 ms │ 5211 ms │
└─────────┴─────────┴─────────┴─────────┴─────────┴────────────┴───────────┴─────────┘
┌───────────┬─────┬──────┬─────────┬─────────┬───────────┬──────────┬────────┐
│ Stat      │ 1%  │ 2.5% │ 50%     │ 97.5%   │ Avg       │ Stdev    │ Min    │
├───────────┼─────┼──────┼─────────┼─────────┼───────────┼──────────┼────────┤
│ Req/Sec   │ 0   │ 0    │ 22,943  │ 23,599  │ 18,744.95 │ 6,995.89 │ 1,081  │
├───────────┼─────┼──────┼─────────┼─────────┼───────────┼──────────┼────────┤
│ Bytes/Sec │ 0 B │ 0 B  │ 7.81 MB │ 8.04 MB │ 6.38 MB   │ 2.38 MB  │ 368 kB │
└───────────┴─────┴──────┴─────────┴─────────┴───────────┴──────────┴────────┘
Req/Bytes counts sampled once per second.
# of samples: 60
1151k requests in 60.02s, 383 MB read
26k errors (0 timeouts)
B Results: perf/load/example/results/result-1757081805041.json
==============================================================
┌─────────┬─────────┬─────────┬─────────┬─────────┬────────────┬───────────┬─────────┐
│ Stat    │ 2.5%    │ 50%     │ 97.5%   │ 99%     │ Avg        │ Stdev     │ Max     │
├─────────┼─────────┼─────────┼─────────┼─────────┼────────────┼───────────┼─────────┤
│ Latency │ 1299 ms │ 1338 ms │ 2003 ms │ 2516 ms │ 1393.62 ms │ 196.76 ms │ 2914 ms │
└─────────┴─────────┴─────────┴─────────┴─────────┴────────────┴───────────┴─────────┘
┌───────────┬─────┬──────┬─────────┬─────────┬───────────┬──────────┬────────┐
│ Stat      │ 1%  │ 2.5% │ 50%     │ 97.5%   │ Avg       │ Stdev    │ Min    │
├───────────┼─────┼──────┼─────────┼─────────┼───────────┼──────────┼────────┤
│ Req/Sec   │ 0   │ 0    │ 19,359  │ 19,935  │ 18,376.02 │ 4,066.35 │ 2,805  │
├───────────┼─────┼──────┼─────────┼─────────┼───────────┼──────────┼────────┤
│ Bytes/Sec │ 0 B │ 0 B  │ 6.59 MB │ 6.79 MB │ 6.26 MB   │ 1.38 MB  │ 955 kB │
└───────────┴─────┴──────┴─────────┴─────────┴───────────┴──────────┴────────┘
Req/Bytes counts sampled once per second.
# of samples: 60
1129k requests in 60.02s, 375 MB read
26k errors (0 timeouts)
Comparison: equal
=================
{
  diff: { rps: '2.01%', throughput: '2.01%', latency: '-2.54%' },
  a: {
    avgRPS: 18744.95,
    avgThroughput: 6382649.6,
    avgLatency: 1358.19,
    status: { '200': { count: 1124707 } }
  },
  b: {
    avgRPS: 18376.02,
    avgThroughput: 6256686.94,
    avgLatency: 1393.62,
    status: { '200': { count: 1102508 } }
  }
}

Notes for folks on these results: The tooling runs while capturing perf events and with the inspector protocol on, it is more designed for debugging performance than benchmarking but it should be pretty reliable for doing comparisons over small patches like this.

Just for context, even for Fastify we get a lot of people wanting to have a body in GET requests unfortunately

Same on Express. Although I would love to make this default and have an option to allow GET with body.

pimterry · 2025-09-05T16:57:16Z

One option is to check the content-length header and only apply this optimization if it's not 0?

We'd also need to check for transfer-encoding with chunked as well, but yes sniffing for this doesn't seem unreasonable to me and should be safe... The RFC lists all the possibilities here, and says you can send response bodies with neither header (in which case body is everything until the end of the connection) but not request bodies.

ronag added the http Issues or PRs related to the http subsystem. label Sep 3, 2025

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 3, 2025

ronag requested review from RafaelGSS and mcollina September 3, 2025 15:45

mcollina approved these changes Sep 3, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2025

This comment was marked as outdated.

Sign in to view

RafaelGSS added the performance Issues and PRs related to the performance of Node.js. label Sep 3, 2025

ronag marked this pull request as ready for review September 3, 2025 20:21

ronag requested a review from mcollina September 3, 2025 20:22

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2025

ronag force-pushed the faster-dump branch 4 times, most recently from d3ff5f6 to e86d740 Compare September 3, 2025 20:27

ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 3, 2025

ronag force-pushed the faster-dump branch from e86d740 to 36ec4ee Compare September 3, 2025 20:29

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 3, 2025

ronag force-pushed the faster-dump branch 5 times, most recently from 197ba2c to 60e12ac Compare September 3, 2025 21:06

Flarna reviewed Sep 3, 2025

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

Flarna reviewed Sep 3, 2025

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

RafaelGSS closed this Sep 4, 2025

RafaelGSS reopened this Sep 4, 2025

This comment was marked as outdated.

Sign in to view

lpinca reviewed Sep 4, 2025

View reviewed changes

ronag force-pushed the faster-dump branch 2 times, most recently from d9d8fbc to fc3f493 Compare September 4, 2025 20:05

ronag added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2025

http: optimize IncomingMessage._dump

04da724

ronag force-pushed the faster-dump branch from fc3f493 to 04da724 Compare September 4, 2025 20:09

O4FDev mentioned this pull request Sep 5, 2025

feat: dump bodies of get/head reqs expressjs/express#6742

Draft

gurgunday reviewed Sep 5, 2025

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

gurgunday approved these changes Sep 5, 2025

View reviewed changes

Flarna reviewed Sep 5, 2025

View reviewed changes

ronag marked this pull request as draft September 5, 2025 08:46

Apply suggestion from @gurgunday

0fe6da1

Co-authored-by: Gürgün Dayıoğlu <[email protected]>

RafaelGSS mentioned this pull request Sep 5, 2025

http: add optimizeEmptyRequests option for IncomingMessage._dump #59778

Open

ronag closed this Sep 6, 2025

Uh oh!

http: optimize IncomingMessage._dump #59747

http: optimize IncomingMessage._dump #59747

Conversation

ronag commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 3, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

github-actions bot commented Sep 3, 2025

Uh oh!

Uh oh!

Uh oh!

ronag commented Sep 4, 2025

Uh oh!

RafaelGSS commented Sep 4, 2025

Uh oh!

This comment was marked as outdated.

lpinca Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

ronag Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

ronag commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 4, 2025

Uh oh!

ronag commented Sep 4, 2025

Uh oh!

RafaelGSS commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Flarna Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

ronag Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Flarna Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

Flarna commented Sep 5, 2025

Uh oh!

ronag commented Sep 5, 2025

Uh oh!

pimterry commented Sep 5, 2025

Uh oh!

gurgunday commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleytodd commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pimterry commented Sep 5, 2025

Uh oh!

Uh oh!

ronag commented Sep 3, 2025 •

edited

Loading

ronag commented Sep 4, 2025 •

edited

Loading

RafaelGSS commented Sep 4, 2025 •

edited

Loading

gurgunday commented Sep 5, 2025 •

edited

Loading

ronag commented Sep 5, 2025 •

edited

Loading

wesleytodd commented Sep 5, 2025 •

edited

Loading