Repository navigation
discussion: can http-parser be replaced with llhttp in LTS (10.x and 12.x)? #31441
Description
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
Activity
mcollina commentedon Jan 21, 2020
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-parserhave the same quirks of http-parser? In case, can we consider shipping llhttp with--insecure-http-parserturned on in v10.x?sam-github commentedon Jan 21, 2020
@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:
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 commentedon Jan 21, 2020
It's been several months since initial release of
llhttpas a default parser in v13. To the best of my knowledge, there are no significant behavior deviations when compared tohttp_parser.I agree with @mcollina's compromise proposal. Moving v12 to
llhttpwhile keeping v10 onhttp_parseris reasonable.sam-github commentedon Jan 21, 2020
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=legacycan speak up so we can fix any issues they have.richardlau commentedon Jan 22, 2020
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 commentedon Jan 22, 2020
Would it be too intrusive to add a deprecation to
--http-parser=legacyin 12.x that requests that people using it report why to nodejs, so we can make llhttp work for their use case?richardlau commentedon Jan 22, 2020
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 commentedon Jan 22, 2020
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 commentedon Jan 22, 2020
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 commentedon Jan 23, 2020
I'm +1 with a deprecation warning that includes the cutoff date and
--insecure-http-parser.sam-github commentedon Feb 4, 2020
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-parseroption. 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 commentedon Feb 4, 2020
@sam-github FWIW, with the exception of few outliers, most of http-parser tests were ported to llhttp.
sam-github commentedon Feb 13, 2020
user converting sucessfully from http-parser to llhttp in insecure mode: #27711 (comment)
sam-github commentedon Feb 14, 2020
FYI: I took a shot at running the http-parser tests against llhttp: nodejs/llhttp#46
BethGriggs commentedon Mar 4, 2021
Stumbled upon this issue. @mcollina do you still think it's appropriate to follow the proposal?
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).
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 commentedon Mar 4, 2021
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.
http: runtime deprecate legacy HTTP parser
richardlau commentedon Mar 30, 2021
Node.js 12.22.0 has been released with the added legacy HTTP parser deprecation and notable changes pointing out the
--insecure-http-parseroption: https://nodejs.org/en/blog/release/v12.22.0/mcollina commentedon Mar 30, 2021
I'm going to close this issue as I think everything has been done.
brody2consult commentedon Mar 31, 2021
nit: I think the link should be changed to this: https://github.com/nodejs/Release#release-phases
to quote: