Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: make parser choice a runtime flag #24739

Closed
wants to merge 5 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 30, 2018

Add a --http-parser=llhttp vs --http-parser=legacy
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

Refs: #24730

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

@addaleax addaleax added http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Nov 30, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 30, 2018
@addaleax
Copy link
Member Author

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

@nodejs/http @nodejs/http-parser

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

doc/api/cli.md Outdated Show resolved Hide resolved
lib/_http_common.js Outdated Show resolved Hide resolved
src/node_http_parser_llhttp.cc Show resolved Hide resolved
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Does lib/internal/print_help.js also need to be updated?

@addaleax
Copy link
Member Author

@cjihrig No, it takes its data from the options list itself. :)

@indutny
Copy link
Member

indutny commented Nov 30, 2018

@addaleax 🤗

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very clever!

@indutny
Copy link
Member

indutny commented Nov 30, 2018

Might have merge conflicts with #24738

@ofrobots
Copy link
Contributor

Continuing from this comment I'm -1 on this flag.

Switching from http_parser to llhttp is not semver major from an API point of view, but since the implementation changes substantially the changeover should happen in a major release from a defensiveness perspective.

Removing this flag, OTOH, will be semver major. Unless the plan is to remove this flag before Node 12 is released, we will have to wait 2 semver majors to remove this flag (and hence remove http_parser).

@addaleax
Copy link
Member Author

addaleax commented Nov 30, 2018

@ofrobots Thanks for weighing in – If this really becomes problematic (i.e. http_parser becomes too much of a burden to maintain), we could make the flag a no-op, and use llhttp regardless of what was passed on the command line, if we’re sufficiently satisfied with the results?

On the other hand, introducing this flag now would give us another 4 – 5 months of allowing (easier) ecosystem testing as an opt-in switch?

@joyeecheung
Copy link
Member

joyeecheung commented Nov 30, 2018

Maybe instead of making the legacy parser switch a noop in the future, we only introduce --experimental-http-parser as a runtime (boolean) switch? It seems to be a bit more consistent with how we introduced the WHATWG URL parser (although you can explicitly opt-in to use that one in the code). Also, experimental features are not part of the LTS - as I understand, we can make experimental features a a noop during LTS, if it's necessary.

If we keep it as a build-time flag, I doubt how we can reach to a point where it's safe enough to ship it without enough experimentation in the user land.

@addaleax
Copy link
Member Author

addaleax commented Dec 1, 2018

@joyeecheung I think that would reduce our ability to provide the legacy parser for as long as it makes sense... I guess that would be okay, but I'm not quite convinced we need to do that?

<!-- YAML
added: REPLACEME
-->

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we should mark this flag itself experimental?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any kind of actual breaking changes that we could make to it? I wouldn’t consider making it a no-op a breaking change. I’d be happy to add a warning about that (possibly no-op-ing it) here, though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning is that the part being experimental is "the ability to choose your HTTPParser". So, while this flag is experimental, the user cannot always expect that they have the ability to choose that, and the flag can be a noop in a particular release (without going semver-major) because of security reasons, which I think doesn't matter that much for people who do experiment with it, and they will be given proper warning about this fact.

When we are confident enough we will just remove the old parser along with this flag, or make the flag a noop and deprecate it. Since in our stability contract, being experimental means the feature itself ("the ability to choose your HTTPParser") may be removed completely. The ability itself will not actually graduate into stable - it will be first experimental and then straight into deprecation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds a bit complex, imo? Wouldn't something "This flag is likely to become a no-op and later be removed at some point in the future" be explicit enough + do the trick?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax What's the difference between that and making this experimental? In my understanding, being experimental means: a flag + a warning when it's used + an orange banner in the documentation + freedom to break it (including making it a noop) in a release line, which I think is what we want for this? (Or maybe my understanding of the experimental status is wrong, or my understanding of your reasoning is different from what you have in mind?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung The difference is that experimental usually implies that people should be careful with using it in production, which doesn’t apply here, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax I see. I am tagging this tsc-review (I don't think tagging it tsc-agenda and discussing it in a meeting would help?) so we can fully discuss about the status of this flag, just to make sure we don't overlook something before pushing it out to users..

Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

Refs: nodejs#24730
@addaleax
Copy link
Member Author

addaleax commented Dec 4, 2018

I’ve rebased this.

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

@ofrobots Could you take another look, esp. at the added warning in the CLI docs (ff9d9cc)?

@joyeecheung
Copy link
Member

joyeecheung commented Dec 4, 2018

I've tagged this tsc-review to see if anyone from @nodejs/tsc has more feedback about the stability status of this flag. Specifically, do we want to signal to the users that this is a flag that can be used in production (in LTS)? What is our expectation regarding the migration/deprecation of this flag? Do we want to make this flag experimental?

@addaleax 's take is that we don't need to discourage users to use it in production so it doesn't need to be experimental. I am currently inclined to making this flag experimental first just so that it fits better into our stability model, but I don't feel very strongly about that.

See #24739 (review) for discussion.

@addaleax
Copy link
Member Author

addaleax commented Dec 4, 2018

Fwiw, what I have in mind is:

  • We introduce this flag now, and switch the default to llhttp in Node 12.
  • Once we want to drop http_parser and are sufficiently convinced that llhttp doesn’t break anybody, we just make it a no-op (as semver-patch) and at least remove it from --help.
  • We’d then fully remove it in some later major version (with no urgency, since it’s a no-op and thus pretty cheap to support).

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2018

Had a linter failure after addressing a comment, sorry.

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

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2018

@Trott
Copy link
Member

Trott commented Dec 5, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19236/ ✔️

@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2018

@Trott There was already a resume CI scheduled, it seems to be blocked on only 1 AIX machine being available I think? Did I miss something?

@Trott
Copy link
Member

Trott commented Dec 5, 2018

Did I miss something?

@addaleax No, all my fault. I saw the failure in the CI interface and resumed without looking to see if one was already running. I'll try to do better.

@danbev
Copy link
Member

danbev commented Dec 6, 2018

Landed in aa943d0.

@danbev danbev closed this Dec 6, 2018
danbev pushed a commit that referenced this pull request Dec 6, 2018
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@addaleax addaleax deleted the llhttp-runtime-switch branch December 6, 2018 09:23
addaleax added a commit to addaleax/node that referenced this pull request Dec 6, 2018
BridgeAR pushed a commit that referenced this pull request Dec 6, 2018
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
BridgeAR added a commit that referenced this pull request Dec 6, 2018
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
@BridgeAR BridgeAR mentioned this pull request Dec 6, 2018
4 tasks
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
BridgeAR pushed a commit that referenced this pull request Dec 7, 2018
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: #24739
Refs: #24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
BridgeAR added a commit that referenced this pull request Dec 7, 2018
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
addaleax added a commit that referenced this pull request Dec 9, 2018
Refs: #24739
Fixes: #24730

PR-URL: #24870
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Add a `--http-parser=llhttp` vs `--http-parser=traditional`
command line switch, to make testing and comparing the new
llhttp-based implementation easier.

PR-URL: nodejs#24739
Refs: nodejs#24730
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Refs: nodejs#24739
Fixes: nodejs#24730

PR-URL: nodejs#24870
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet