discussion: can http-parser be replaced with llhttp in LTS (10.x and 12.x)?

[sam-github]

Maintenance of nodejs/http-parser has not gone away, it continues to require bug fixing, even though it was removed in 14.x/master. This is an ongoing maintenance burden, and will continue until 12.x is EOL in late 2021.

Is it possible to replace the http-parser with llhttp in LTS? Originally, it was considered risky. llhttp was new, its behaviour had not been field tested. At this point, its been in use a while, has been robust. It has had bugs, but those bugs have often been found to be present in both parsers, and have had to be fixed in both.

It seems to an arms-length observer that the http-parser and https://github.com/nodejs/llhttp are being kept very close in terms of feature compatibility, even to the introduction of legacy "lax parsing" modes for both of them (see #30567 and related).

If it is possible in insecure mode for every HTTP connection supported by http-parser to also be supported by llhttp, I think we should consider what it would take to confidently drop llhttp into http-parser's place in 10.x and make --http-parser=legacy either a no-op in 12.x, or an alias for --insecure-http-parser.

So, what would it take? Are there some specific things we can do to move towards replacement? Is it out of the question? Are they already compatible enough?

to: @indutny @nodejs/lts @nodejs/tsc @nodejs/http

[mcollina]

Quick errata: 10.x will go EOL on April 2021, while 12.x will go EOL on April 2022.

From my experience in dealing with production system, this seems a very dangerous proposition for our backward compatibility story. Changing the http parser is a breaking change, and the TSC can decide to ship a breaking change in LTS if we think it is important.

I am +1 in removing http-parser from 12.x with the introduction of --insecure-http-parser, as it seems to have fixed most of the issues in the migration. Removing it from 12.x will shorten our support window by one year, and I think it'd be a good compromise.

I'm more conflicted about what to do with 10.x. Do --insecure-http-parser have the same quirks of http-parser? In case, can we consider shipping llhttp with --insecure-http-parser turned on in v10.x?

[sam-github]

@mcollina thanks for correcting the EOL dates, I misread the graph in https://github.com/nodejs/Release#release-schedule, I didn't notice it was cut off on the right side!

I'm tentative about this, too, I don't want to give the impression I'm all gung-ho, but that said:

Changing the http parser is a breaking change

Not unless it actually breaks something.... and we don't know it does.

Any change can inadvertently introduce an unwanted effect. That doesn't make it semver-major, it makes it a bug in that change. A bug that can be fixed.

[indutny]

It's been several months since initial release of llhttp as a default parser in v13. To the best of my knowledge, there are no significant behavior deviations when compared to http_parser.

I agree with @mcollina's compromise proposal. Moving v12 to llhttp while keeping v10 on http_parser is reasonable.

[sam-github]

Moving v12 to llhttp while keeping v10 on http_parser is reasonable.

Note that 12.x is currently defaulting to llhttp (but http-parser is selectable with --http-parser=legacy).

As long as we support 10.x, we have to update http-parser when there are HTTP problems, so removing it from 12.x doesn't save much. Updating 12 is usually just a drop-in.

I propose we EOL http-parser support in Node.js 12.x when we EOL Node.js 10.x, April 2021.

That's not a very short timeline, but it does remove 1 year from the http-parser support lifetime, which would otherwise be until EOL of 12x, April 2022.

We can start communicating that now, so no one is surprised, and hopefully anyone who is relying on using --http-parser=legacy can speak up so we can fix any issues they have.

[richardlau]

Under normal circumstances if you want to get llhttp into 10.x as an option it would have to be soonish as there's only one scheduled semver-minor left before it goes into maintenance (nodejs/Release#504). http-parser would need to remain the default.

The exception would be if a case was made on security grounds as we have policy allowing for breaking changes for security reasons.

[sam-github]

Would it be too intrusive to add a deprecation to --http-parser=legacy in 12.x that requests that people using it report why to nodejs, so we can make llhttp work for their use case?

[richardlau]

An actual produces warning deprecation (as opposed to docs-only deprecation)? I'm not sure what the policy for introducing those in a release line is (I think we've done it before but I can't remember what for).

[sam-github]

Yes, an actual console.error() output.... or perhaps a 'warning' that is not a deprecation.

I'm usually pretty sensitive to adding this kind of output to LTS, but in this case, it will only be seen when people are already doing something unusual as a work-around for llhttp not working for their use case. Particularly if we use their reports to update llhttp to supports their use case (either in normal or lenient aka "insecure" mode), I hope they would be not be so annoyed. @nodejs/lts

[sam-github]

And to put it another way.... if we do not do this, then once we replace llhttp with http-parser in LTS, they will be stuck with no fix until we update llhttp, perhaps they'd rather get a console message while waiting for llhttp to be updated than simply not have a work-around at all?

Well, other than not updating, I guess that workaround is always available. But that works for the deprecation message, too, they can report the bug, not update, we fix, then they update and won't see the deprecation message because they won't have to use the old parser.

[mcollina]

I'm +1 with a deprecation warning that includes the cutoff date and --insecure-http-parser.

[sam-github]

For 10.x, how would @nodejs/lts and @nodejs/http feel if I rworked the v10.x/deps/http-parser tests so they can be run with llhttp?

That would give even more confidence that llhttp is behaviour compatible with the http-parser (or perhaps find variances). If there are inadvertent variances, they can be fixed. If there are deliberate changes (probably strictness / correctness), then we can make sure they can be disabled with the --insecure-http-parser option. Maybe its just the pain of the recent maintenance, but I'd really like us to get out of maintaining http-parser, and we can't do that as long as its in any LTS release line.

If replacing http-parser with llhttp in v10.x is a hard "no", even if it passes the node and http-parser test suites, then I won't spend the time on it, so I'd like to know its a reasonable route to take before starting the work.

cc: @indutny

[indutny]

@sam-github FWIW, with the exception of few outliers, most of http-parser tests were ported to llhttp.

[sam-github]

user converting sucessfully from http-parser to llhttp in insecure mode: #27711 (comment)

[sam-github]

FYI: I took a shot at running the http-parser tests against llhttp: nodejs/llhttp#46

[BethGriggs]

Stumbled upon this issue. @mcollina do you still think it's appropriate to follow the proposal?

I propose we EOL http-parser support in Node.js 12.x when we EOL Node.js 10.x, April 2021.

With the proposed cutoff date being April 2021, I do have mild concerns (not blocking) that it is a little late to add the deprecation warning (especially during maintenance).

+1 with a deprecation warning that includes the cutoff date and --insecure-http-parser.

If we want to go ahead with the warning in Node.js 12, I think it makes sense to try and include that in the next release (ideally we'd do the release before the end of April).

[mcollina]

I think we should ship the warning. As far as removal goes, we can keep it here in v12 for the time being and just remove it on the first security vulnerability.

Fixing security issues in http-parser is incredibly hard and there is a non-zero chance that the fixing http_parser is impractical within the allowed time period. The last time I had to work on it, it took me a week worth of work to fix the problem.

[richardlau]

Node.js 12.22.0 has been released with the added legacy HTTP parser deprecation and notable changes pointing out the --insecure-http-parser option: https://nodejs.org/en/blog/release/v12.22.0/

[mcollina]

I'm going to close this issue as I think everything has been done.

[brodycj]

[...]
we have policy allowing for breaking changes for security reasons.

nit: I think the link should be changed to this: https://github.com/nodejs/Release#release-phases

to quote:

Changes required for critical security and bug fixes may lead to semver-major
changes landing within a release stream, such situations will be rare and will
land as semver-minor.