http: remove legacy parser

[addaleax]

Remove the legacy http_parser implementation as a dependency
and all code that uses it in favor of llhttp, given that the latter
has been the default for all of Node 12 with no outstanding issues.

---

I think one of the ideas behind making llhttp the default for v12.x is that that means that we could drop http_parser for Node 12 LTS, reducing the time that it needs to be supported. In that case, this should probably land soon-ish. I’m personally also okay with landing this before or after Node 13 only, if people prefer that.

@nodejs/tsc @nodejs/http

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

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

[mcollina]

LGTM

I think this is semver-major.

[addaleax]

I think this is semver-major.

@mcollina Do you mind explaining why? This doesn’t come with API changes, neither for the http module nor CLI flags, it only changes llhttp from being the default to being always-on.

[mcollina]

We used to have a flag to switch between the two, have you removed that?

[addaleax]

@mcollina It’s turned into a no-op here in order to make this as un-breaking as possible :)

[mcollina]

@mcollina It’s turned into a no-op here in order to make this as un-breaking as possible :)

If somebody is relying on the legacy parser behavior, this would break them. I think it's better to remove the flag and call it semver-major.

[addaleax]

If somebody is relying on the legacy parser behaviour in some way, then yes, this is semver-major – I’d consider the chances of that actually being the case pretty slim, though.

I wouldn’t want to remove the flag in this PR either way. (Honestly, I personally wouldn’t want to remove any flags that can reasonably be turned into no-ops, ever.) That can happen later and seems like a different concern to me.

[addaleax]

Anyway, if you think this is semver-major, feel free to add the label. As the PR description says, I’m fine with that, even if I don’t think it’s necessary.

[mcollina]

I'm tagging this tsc-agenda, just to poll the @nodejs/tsc. If the TSC has no objection, I'm happy for a semver-minor.

[sam-github]

I think removing a feature is semver-major, so removing http_parser is semver-major. There have been differences between parsers noticed in the past, being careful here seems a good thing to me.

Removing the CLI option would also be semver-major, even if it is a no-op.

Working around different CLI options for different node.js versions is difficult, so I'm in favour of leaving the CLI option to select the http_parser around even after the parser is gone, though perhaps it should print a warning saying that it isn't actually doing anything?

[jasnell]

hmm... tough one. I can see your rationale on this for semver-minor @addaleax. That said, our deprecation policy requires that End-of-life transitions are semver-major, and there's a likely-very-small-but-still-non-zero possibility that someone is building a native addon somewhere that depends on the presence of the http_parser.h headers, regardless of whether they are depending on any of the actual behaviors implemented. I think to be safe we have to consider this a semver-major change.

[jasnell]

Marking semver-major for now, but this should definitely be discussed on the TSC meeting.

[lpinca]

RSLGTM

[lpinca]

There seem to be a spurious commit was this done on purpose?

[jasnell]

LGTM as a semver-major (and with the extraneous commit removed)

[addaleax]

I think removing a feature is semver-major, so removing http_parser is semver-major.

I don’t think this qualifies as a feature, it’s a change in implementation details. The only question is whether it’s a big enough one to have to be more careful than usual here.

There seem to be a spurious commit was this done on purpose?

That seems to be @Trott rebasing nodejs/master. I’ve rebased this against master again, the extra commit is gone.

That said, our deprecation policy requires that End-of-life transitions are semver-major

(I don’t really understand why this was deprecated, either.)

and there's a likely-very-small-but-still-non-zero possibility that someone is building a native addon somewhere that depends on the presence of the http_parser.h headers, regardless of whether they are depending on any of the actual behaviors implemented.

We don’t publish the http parser headers along with Node, and we don’t re-export the symbols on Windows. I think it’s safe to say that this isn’t a concern here.

[jasnell]

We don’t publish the http parser headers along with Node, and we don’t re-export the symbols on Windows. I think it’s safe to say that this isn’t a concern here.

👍

Ok, well, I'm good with the change overall. If there are no objections from the rest of the TSC on dropping the semver-major on this, then I'm good with that also, but as it is right now because of the deprecation it's technically semver-major :-/