Review requested:

• @nodejs/http
• @nodejs/net

Good work!

The problematic parser->current_buffer_.IsEmpty() check is a speed hack; it's explicitly documented as such. Adding a hack to work around a hack is not a good idea.

My suggestion: remove current_buffer_. That simplifies the parser and makes it re-entrant again. Worry about performance afterwards.

I speculate the hit won't be so bad, and can probably be fixed by doing the bookkeeping in JS land.

@bnoordhuis Are you suggesting to remove that just in Execute, isn't it?

current_buffer_ is used to thread through the buffer from a parser.execute(buffer) call from JS through C++ to the JS callback parser[kOnBody] = parserOnBody(buffer, start, len) { /* ... */ }.

That threading through isn't necessary. Just set parser.buffer = buffer before calling parser.execute(buffer) and the callback can access it as this.buffer.

Node can short-circuit and pass data directly from libuv to the callback without going through parser.execute(buffer) first but the callback can detect that by doing buffer = this.buffer ?? buffer.

OnStreamRead()should be modified to create a new buffer instead of reusing current_buffer_. In theory that's a performance hit but I expect it's a wash because the callback no longer needs to call buffer.slice().

If I had infinite free time, I'd rewrite the parser to get rid of C++ -> JS callbacks and just return parse info. It'd get rid of so much complexity and inefficiency.

Actually I was able to remove the current_buffer_ at all and apparently nothing changes.
The only relevant change is in on_body where I always do:

Local<Object> current_buffer_ = scope.Escape(Buffer::Copy(
        env()->isolate(),
        current_buffer_data_,
        current_buffer_len_).ToLocalChecked());

Do you see any problem in this?

@addaleax @bnoordhuis I've sent my latest version which removes current_buffer_ and thus also removes needs for all the suggestion Anna asked about. Can you please take a look again?

@bnoordhuis I applied your changes (plus few additional required changes). How does it look now?

Is there a reason you didn't update lib/_http_common.js? You don't need to slice anymore, or pass the start and len arguments.

@bnoordhuis The answer is obvious: I forgot. X°°°D Done now!

@bnoordhuis The last item would be to try to test it without using the parser directly. I'll give it a try today but this can go out without as well.

Done!

Thanks sir :)

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

@nodejs/http Can anybody reapprove this so I can land it?

Landed in b970634

Hello @ShogunPanda

Thanks for your work !
Can you please tell me in which version of NodeJS is it fixed please ?

@j-catania It was merged in main, which means it will be available in the next 18 release, I think 18.5.0. Not sure it will be backported.

Is there a way to help see this backported to 16.x? This bug is really biting us as we are doing file uploads, and the 18 LTS release is still ~4 months off so upgrading isn't an option for us, and that we expect surrounding infra will take a few months to add 18 support after it releases.

Is there a way to help see this backported to 16.x? This bug is really biting us as we are doing file uploads, and the 18 LTS release is still ~4 months off so upgrading isn't an option for us, and that we expect surrounding infra will take a few months to add 18 support after it releases.

@nodejs/tsc Can we do this? What is the policy about backporting?

Is there a way to help see this backported to 16.x? This bug is really biting us as we are doing file uploads, and the 18 LTS release is still ~4 months off so upgrading isn't an option for us, and that we expect surrounding infra will take a few months to add 18 support after it releases.

@nodejs/tsc Can we do this? What is the policy about backporting?

Standard policy is:

• Semver patch commits can be backported to LTS although we prefer to have them go have been in a current release for at least two weeks first (https://github.com/nodejs/Release#lts-staging-branches).
• Semver minor changes, in addition to the "two weeks in current" guideline, would wait for the next semver minor LTS release (generally one per quarter).

Thanks both.
In other words, in this particular case, I think the change is semver-patch. So I will be able to send a backport PR in about two weeks from the release of 18.5.0. Am I right?

If the commit lands cleanly on v16.x-staging, there's no need for a backport PR:

If a backport PR is necessary, you don't need to wait two weeks for opening it, but the LTS team won't land it until it's been on a Current version for at least two weeks.

I see.
Given that you added the label now, the tooling will try to perform the landing automatically, isn't it?

No, someone from @nodejs/lts will (try to) do it when preparing the next release, or leave a comment here if there's a conflict.