http: defer reentrant execution of Parser::Execute

[ShogunPanda]

This PR fixes an edge case in which Parser::Execute is called within the callback of one of its events.

Fixes: #39671

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mcollina]

Good work!

[bnoordhuis]

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.

[ShogunPanda]

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

[bnoordhuis]

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.

[ShogunPanda]

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?

[ShogunPanda]

@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?

[ShogunPanda]

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

[bnoordhuis]

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.

[ShogunPanda]

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

[bnoordhuis]

Nice, good work!

@addaleax I'll go ahead and dismiss your review. Looks like all actionable feedback's been addressed.

obsolete

[ShogunPanda]

@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!

[mcollina]

lgtm

wow, when a PR fixes a bug and reduces code, it's doing something right.

[ShogunPanda]

Thanks sir :)

[nodejs-github-bot]

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

[ShogunPanda]

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

[mcollina]

lgtm

[ShogunPanda]

Landed in b970634

[j-catania]

Hello @ShogunPanda

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

[ShogunPanda]

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

[MasterOdin]

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.

[ShogunPanda]

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?

[aduh95]

node/doc/contributing/backporting-to-release-lines.md

Lines 19 to 25 in 7bf50bc

	## What can be backported?
	The "Current" release line is much more lenient than the LTS release lines in
	what can be landed. Our LTS release lines (see the [Release Plan][])
	require that commits mature in the Current release for at least 2 weeks before
	they can be landed in an LTS staging branch. Only after "maturation" will those
	commits be cherry-picked or backported.

[richardlau]

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

[ShogunPanda]

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?

[aduh95]

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

node/doc/contributing/backporting-to-release-lines.md

Lines 11 to 17 in 7bf50bc

	## What needs to be backported?
	If a cherry-pick from `main` does not land cleanly on a staging branch, the
	releaser will mark the pull request with a particular label for that release
	line (e.g. `backport-requested-vN.x`), specifying to our tooling that this
	pull request should not be included. The releaser will then add a comment
	requesting that a backport pull request be made.

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.

[ShogunPanda]

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

[aduh95]

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.