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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

http: switch default parser to llhttp #24870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)')
Copy link
Member Author

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.

Copy link
Member

@joyeecheung joyeecheung Dec 6, 2018 •
edited
Loading

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.

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

Copy link
Member

@joyeecheung joyeecheung Dec 6, 2018 •
edited
Loading

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

Copy link
Member Author

@addaleax addaleax Dec 6, 2018 •
edited
Loading

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?


shared_optgroup.add_option('--shared-http-parser',
action='store_true',
Expand Down Expand Up @@ -1117,9 +1117,6 @@ def configure_node(o):
else:
o['variables']['node_target_type'] = 'executable'

o['variables']['node_experimental_http_parser'] = \
b(options.experimental_http_parser)

def configure_library(lib, output):
shared_lib = 'shared_' + lib
output['variables']['node_' + shared_lib] = b(getattr(options, shared_lib))
Expand Down
2 changes: 1 addition & 1 deletion doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ Chooses an HTTP parser library. Available values are:
- `llhttp` for https://llhttp.org/
- `legacy` for https://github.com/nodejs/http-parser

The default is `legacy`, unless otherwise specified when building Node.js.
The default is `llhttp`, unless otherwise specified when building Node.js.

This flag exists to aid in experimentation with the internal implementation of
the Node.js http parser.
Expand Down
1 change: 0 additions & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
'force_dynamic_crt%': 0,
'node_module_version%': '',
'node_shared_zlib%': 'false',
'node_experimental_http_parser%': 'false',
'node_shared_http_parser%': 'false',
'node_shared_cares%': 'false',
'node_shared_libuv%': 'false',
Expand Down
4 changes: 0 additions & 4 deletions node.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,6 @@
],
}],

[ 'node_experimental_http_parser=="true"', {
'defines': [ 'NODE_EXPERIMENTAL_HTTP_DEFAULT' ],
} ],

[ 'node_shared_http_parser=="false"', {
'dependencies': [
'deps/http_parser/http_parser.gyp:http_parser',
Expand Down
12 changes: 0 additions & 12 deletions src/inspector_socket.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
#include "inspector_socket.h"

#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
#define NODE_EXPERIMENTAL_HTTP
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Sure!

#endif
#include "http_parser_adaptor.h"

#include "util-inl.h"
Expand Down Expand Up @@ -437,13 +435,8 @@ class HttpHandler : public ProtocolHandler {
explicit HttpHandler(InspectorSocket* inspector, TcpHolder::Pointer tcp)
: ProtocolHandler(inspector, std::move(tcp)),
parsing_value_(false) {
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
llhttp_init(&parser_, HTTP_REQUEST, &parser_settings);
llhttp_settings_init(&parser_settings);
#else /* !NODE_EXPERIMENTAL_HTTP_DEFAULT */
http_parser_init(&parser_, HTTP_REQUEST);
http_parser_settings_init(&parser_settings);
#endif /* NODE_EXPERIMENTAL_HTTP_DEFAULT */
parser_settings.on_header_field = OnHeaderField;
parser_settings.on_header_value = OnHeaderValue;
parser_settings.on_message_complete = OnMessageComplete;
Expand Down Expand Up @@ -488,17 +481,12 @@ class HttpHandler : public ProtocolHandler {

void OnData(std::vector<char>* data) override {
parser_errno_t err;
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
err = llhttp_execute(&parser_, data->data(), data->size());

if (err == HPE_PAUSED_UPGRADE) {
err = HPE_OK;
llhttp_resume_after_upgrade(&parser_);
}
#else /* !NODE_EXPERIMENTAL_HTTP_DEFAULT */
http_parser_execute(&parser_, &parser_settings, data->data(), data->size());
err = HTTP_PARSER_ERRNO(&parser_);
#endif /* NODE_EXPERIMENTAL_HTTP_DEFAULT */
data->clear();
if (err != HPE_OK) {
CancelHandshake();
Expand Down
4 changes: 0 additions & 4 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,7 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddOption("--expose-internals", "", &EnvironmentOptions::expose_internals);
AddOption("--http-parser",
"Select which HTTP parser to use; either 'legacy' or 'llhttp' "
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
"(default: llhttp).",
#else
"(default: legacy).",
#endif
&EnvironmentOptions::http_parser,
kAllowedInEnvironment);
AddOption("--loader",
Expand Down
7 changes: 1 addition & 6 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,7 @@ class EnvironmentOptions : public Options {
bool experimental_vm_modules = false;
bool experimental_worker = false;
bool expose_internals = false;
std::string http_parser =
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
"llhttp";
#else
"legacy";
#endif
std::string http_parser = "llhttp";
bool no_deprecation = false;
bool no_force_async_hooks_checks = false;
bool no_warnings = false;
Expand Down