http: add optimizeEmptyRequests option for IncomingMessage._dump

[RafaelGSS]

Backportable version of #59747

cc: @ronag

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.26%. Comparing base (737b42e) to head (50a0195).
⚠️ Report is 78 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59778      +/-   ##
==========================================
- Coverage   89.95%   88.26%   -1.69%     
==========================================
  Files         667      702      +35     
  Lines      196776   206717    +9941     
  Branches    38409    39764    +1355     
==========================================
+ Hits       177006   182461    +5455     
- Misses      12198    16271    +4073     
- Partials     7572     7985     +413     

Files with missing lines	Coverage Δ
lib/_http_incoming.js	99.35% <100.00%> (+0.01%)	⬆️
lib/_http_server.js	97.02% <100.00%> (-0.18%)	⬇️

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69091/

[marco-ippolito]

The OR seems redudant since we are passing through validateBoolean

[RafaelGSS]

How so? if fastDump is undefined, this[kfastDump] will be undefined instead of false.

[jasnell]

You could shortcut that just a bit by assigning a default:

const { fastDump = false } = options.fastDump;
validateBoolean(fastDump, 'options.fastDump');
this[kfastDump] = fastDump;

[pimterry]

Thinking more about the heuristics mentioned at the end #59747, it looks like we can go further and do even better here, very easily.

I think we can do this for all methods, quickly & safely, because it's already impossible to receive a request with a body without it being indicated it in the headers (content-length or transfer-encoding), even in the current implementation, even for POST and others etc.

On Node v24 right now, if you send:

POST / HTTP/1.1
Host: test

message body

Then Node currently fires request and end (but no data), and then returns a 400 response to the unparseable message body (which it expects to be $METHOD $PATH HTTP/1.1 starting a new request). That's because the RFC possibilities for sending request bodies always require either content-length or transfer-encoding to be set to warn you there's a body coming, and our parsing already requires this as well.

That means we could safely apply this optimization to all cases, whenever there's no content-length or transfer-encoding header set (we could look for chunked or check for != 0, but since it's primarily a performance optimization I think a quicker simpler check is fine to start with). We already have the headers parsed here so that's very quick & easy. That lets us apply the same performance boost to POSTs and all other requests with an empty body (which is reasonably common I think - there's plenty of REST APIs where the URL defines the operation, but the request is a POST because it's not safe).

This would still be breaking behaviour, because it doesn't emit end or close events, but as an opt-in option (implicitlyCloseEmptyRequests or similar) then that's fine. Or we could check for event listeners and just manually emit the events if any are registered, in which case this wouldn't be breaking at all AFAICT, so we could apply the optimization to everybody for free, but I don't know if the overhead of that negates too much of the perf boost.

[RafaelGSS]

Good one @pimterry. I'll implement those later today and the tests asked by @Flarna.

[RafaelGSS]

Just pushed a new version (amended) that replaces the fastDump with optimizeEmptyRequest option and as suggested by @pimterry, we'll dump the IncomingMessage whenever this option is true and when there's no content-length/transfer-encoding or the method is HEAD/GET.

PTAL.

[Flarna]

Could we move the modifications of stream internals into stream files?
I wonder also that endEmitted/ closeEmitted are set here. Are they really emitted?

[ronag]

I wonder also that endEmitted/ closeEmitted are set here. Are they really emitted?

They are as if they were already emitted.

[RafaelGSS]

Applied the suggestions and added a test to guarantee .end event isn't emitted if the flag is true. I will do a second round of performance comparison on Fastify (Express doesn't seem to use req object as I thought it would use it - so the result wasn't so dramatic).

PTAL @Flarna @pimterry

[Flarna]

@lpinca commented on the previous PR, maybe worth to ask them for a look a this one.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/69182/

[mcollina]

Unfortunately this new option is not easy to turn on by default because it's blocking a valid case, the HTTP/1.0 case where a request is made and the socket half closed to signal its termination. I would rather have it enabled based on the HTTP method instead.