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
base: master
from

Conversation

@addaleax
Copy link
Member

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

This comment has been minimized.

@joyeecheung
Copy link
Member

joyeecheung left a comment

Nice!

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

cjihrig left a comment

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

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Nov 30, 2018

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

@indutny

This comment has been minimized.

Copy link
Member

indutny commented Nov 30, 2018

@addaleax 🤗

@indutny
Copy link
Member

indutny left a comment

LGTM. Very clever!

@indutny

This comment has been minimized.

Copy link
Member

indutny commented Nov 30, 2018

Might have merge conflicts with #24738

@ofrobots

This comment has been minimized.

Copy link
Contributor

ofrobots commented Nov 30, 2018

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

@joyeecheung

joyeecheung Dec 1, 2018

Member

Or maybe we should mark this flag itself experimental?

This comment has been minimized.

@addaleax

addaleax Dec 2, 2018

Member

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?

This comment has been minimized.

@joyeecheung

joyeecheung Dec 2, 2018

Member

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.

This comment has been minimized.

@addaleax

addaleax Dec 2, 2018

Member

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?

This comment has been minimized.

@joyeecheung

joyeecheung Dec 2, 2018

Member

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

This comment has been minimized.

@addaleax

addaleax Dec 4, 2018

Member

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

This comment has been minimized.

@joyeecheung

joyeecheung Dec 4, 2018

Member

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

addaleax added some commits Nov 30, 2018

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

Refs: #24730

@addaleax addaleax force-pushed the addaleax:llhttp-runtime-switch branch from c54665a to ff9d9cc Dec 4, 2018

@addaleax

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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).
@mcollina
Copy link
Member

mcollina left a comment

LGTM

@addaleax

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

addaleax commented Dec 5, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 5, 2018

@addaleax

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

danbev commented Dec 6, 2018

Landed in aa943d0.

@danbev danbev closed this Dec 6, 2018

danbev added a commit that referenced this pull request Dec 6, 2018

http: make parser choice a runtime flag
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 addaleax:llhttp-runtime-switch branch Dec 6, 2018

addaleax added a commit to addaleax/node that referenced this pull request Dec 6, 2018

@addaleax addaleax referenced this pull request Dec 6, 2018

Closed

http: switch default parser to llhttp #24870

3 of 3 tasks complete

BridgeAR added a commit that referenced this pull request Dec 6, 2018

http: make parser choice a runtime flag
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

2018-12-06, Version 11.4.0 (Current)
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 referenced this pull request Dec 6, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 7, 2018

http: make parser choice a runtime flag
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

2018-12-07, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

http: make parser choice a runtime flag
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

2018-12-07, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

http: switch default parser to llhttp
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>

@GulajavaMinistudio GulajavaMinistudio referenced this pull request Dec 10, 2018

Merged

Python statement #375

0 of 4 tasks complete

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

http: make parser choice a runtime flag
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 added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
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 added a commit to refack/node that referenced this pull request Jan 14, 2019

http: switch default parser to llhttp
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