http: switch default parser to llhttp by addaleax · Pull Request #24870 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

addaleax

Refs: #24739
Fixes: #24730

Checklist

@addaleax

@addaleax addaleax added http

Issues or PRs related to the http subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.

http_parser

Issues and PRs related to the HTTP Parser dependency or the http_parser binding.

labels

Dec 6, 2018

@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added c++

Issues and PRs that require attention from people who are familiar with C++.

lib / src

Issues and PRs related to general changes in the lib or src directory.

labels

Dec 6, 2018

@addaleax

addaleax

@@ -187,7 +187,7 @@
parser.add_option('--experimental-http-parser',
action='store_true',
dest='experimental_http_parser',
help='use llhttp instead of http_parser by default')
help='(no-op)')

Choose a reason for hiding this comment

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

Also happy to remove this completely if that’s preferred.

Choose a reason for hiding this comment

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

If we are going to land this patch fairly soon (before April), it's probably easier if we simply flip the default value of the build time switch but do not actually remove it, and postpone the actual removal of the code until we have more usage in the wild.

Choose a reason for hiding this comment

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

@joyeecheung The thing is … is there a way to flip the default value here? How do we do that? I assume we could introduce a new flag --no-experimental-http-parser or so, but that seems … idk?

Choose a reason for hiding this comment

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

Does adding default=True here work? (From what I can tell it should do the trick?)

Also I just notice that we have a bunch of '--shared-http-parser*` flags below, should we change the help text of them as well? (They should be practically turned into noops by this patch since the library is not actually used?) EDIT: oh, nvm, they still work if you use the runtime switch..

Choose a reason for hiding this comment

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

@joyeecheung That would leave us with no way to turn it off (at build time), right? So it’s the same as making this a no-op?

joyeecheung

Choose a reason for hiding this comment

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

OK, let's land this first...if anything should happen, this is probably not too hard to revert on master anyway

@addaleax

@addaleax

trivikr

@@ -1,8 +1,6 @@
#include "inspector_socket.h"
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
#define NODE_EXPERIMENTAL_HTTP

Choose a reason for hiding this comment

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

Where is this value NODE_EXPERIMENTAL_HTTP consumed?
If it's required, should we rename it as llhttp won't be experimental anymore after this commit is pushed?

Choose a reason for hiding this comment

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

It’s consumed in http_parser_adaptor.h.

I agree that the original name was not a great choice, but I am not sure that it would be worth the churn of renaming – if you feel strongly about it, you can feel free to push a new commit to this PR or open a new one afterwards?

Choose a reason for hiding this comment

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

Sure!

@Trott

Needs one more TSC approval. @nodejs/tsc

cjihrig

indutny

Choose a reason for hiding this comment

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

RSLGTM

trivikr

@addaleax

@addaleax addaleax deleted the llhttp-runtime-switch-2 branch

December 9, 2018 21:22

addaleax added a commit that referenced this pull request

Dec 9, 2018

@addaleax

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

@refack

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

BethGriggs added a commit that referenced this pull request

Apr 22, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

BethGriggs added a commit that referenced this pull request

Apr 23, 2019

@BethGriggs

Notable changes:

PR-URL: #26930

This was referenced

Apr 23, 2019

This was referenced

Apr 23, 2019

@hlthi hlthi mentioned this pull request

Apr 25, 2019

Labels

c++

Issues and PRs that require attention from people who are familiar with C++.

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

PRs that contain breaking changes and should be released in the next major version.