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

Jump to bottom

http: make parser choice a runtime flag #24739

Closed
wants to merge 5 commits into from
Closed

http: make parser choice a runtime flag #24739

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3587d21
http: make parser choice a runtime flag
addaleax Nov 30, 2018
9b48c54
squash! http: make parser choice a runtime flag
addaleax Nov 30, 2018
ff9d9cc
fixup! http: make parser choice a runtime flag
addaleax Dec 4, 2018
22f66ba
fixup! http: make parser choice a runtime flag
ofrobots Dec 5, 2018
fc8aa6b
fixup! fixup! http: make parser choice a runtime flag
addaleax Dec 5, 2018
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
2 changes: 1 addition & 1 deletion 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')
help='use llhttp instead of http_parser by default')

shared_optgroup.add_option('--shared-http-parser',
action='store_true',
Expand Down
15 changes: 15 additions & 0 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,21 @@ added: v6.0.0
Force FIPS-compliant crypto on startup. (Cannot be disabled from script code.)
(Same requirements as `--enable-fips`.)

### `--http-parser=library`
<!-- 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

@joyeecheung joyeecheung Dec 2, 2018 •
edited
Loading

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

@joyeecheung joyeecheung Dec 2, 2018 •
edited
Loading

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

@joyeecheung joyeecheung Dec 4, 2018 •
edited
Loading

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

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.

This flag is likely to become a no-op and later be removed at some point
addaleax marked this conversation as resolved.
Show resolved Hide resolved
in the future.

### `--icu-data-dir=file`
<!-- YAML
added: v0.11.15
Expand Down
6 changes: 6 additions & 0 deletions doc/node.1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,12 @@ Enable experimental ES module support in VM module.
.It Fl -experimental-worker
Enable experimental worker threads using worker_threads module.
.
.It Fl -http-parser Ns = Ns Ar library
Chooses an HTTP parser library. Available values are
.Sy llhttp
or
.Sy legacy .
.
.It Fl -force-fips
Force FIPS-compliant crypto on startup
(Cannot be disabled from script code).
Expand Down
4 changes: 2 additions & 2 deletions lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,14 @@
const util = require('util');
const net = require('net');
const url = require('url');
const { HTTPParser } = internalBinding('http_parser');
const assert = require('assert').ok;
const {
_checkIsHttpToken: checkIsHttpToken,
debug,
freeParser,
httpSocketSetup,
parsers
parsers,
HTTPParser,
} = require('_http_common');
const { OutgoingMessage } = require('_http_outgoing');
const Agent = require('_http_agent');
Expand Down
9 changes: 7 additions & 2 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@

'use strict';

const { methods, HTTPParser } = internalBinding('http_parser');
const { getOptionValue } = require('internal/options');

const { methods, HTTPParser } =
getOptionValue('--http-parser') === 'legacy' ?
internalBinding('http_parser') : internalBinding('http_parser_llhttp');

const { FreeList } = require('internal/freelist');
const { ondrain } = require('internal/http');
Expand Down Expand Up @@ -243,5 +247,6 @@ module.exports = {
httpSocketSetup,
methods,
parsers,
kIncomingMessage
kIncomingMessage,
HTTPParser
};
2 changes: 1 addition & 1 deletion lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

const util = require('util');
const net = require('net');
const { HTTPParser } = internalBinding('http_parser');
const assert = require('assert').ok;
const {
parsers,
Expand All @@ -34,6 +33,7 @@ const {
chunkExpression,
httpSocketSetup,
kIncomingMessage,
HTTPParser,
_checkInvalidHeaderChar: checkInvalidHeaderChar
} = require('_http_common');
const { OutgoingMessage } = require('_http_outgoing');
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ const handleConversion = {
if (freeParser === undefined)
freeParser = require('_http_common').freeParser;
if (HTTPParser === undefined)
HTTPParser = internalBinding('http_parser').HTTPParser;
HTTPParser = require('_http_common').HTTPParser;

// In case of an HTTP connection socket, release the associated
// resources
Expand Down
4 changes: 3 additions & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,8 @@
'src/node_encoding.cc',
'src/node_errors.cc',
'src/node_file.cc',
'src/node_http_parser.cc',
'src/node_http_parser_llhttp.cc',
'src/node_http_parser_traditional.cc',
'src/node_http2.cc',
'src/node_i18n.cc',
'src/node_messaging.cc',
Expand Down Expand Up @@ -426,6 +427,7 @@
'src/node_contextify.h',
'src/node_errors.h',
'src/node_file.h',
'src/node_http_parser_impl.h',
'src/node_http2.h',
'src/node_http2_state.h',
'src/node_i18n.h',
Expand Down
14 changes: 8 additions & 6 deletions node.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,14 @@
}],

[ 'node_experimental_http_parser=="true"', {
'defines': [ 'NODE_EXPERIMENTAL_HTTP' ],
'dependencies': [ 'deps/llhttp/llhttp.gyp:llhttp' ],
}, {
'conditions': [ [ 'node_shared_http_parser=="false"', {
'dependencies': [ 'deps/http_parser/http_parser.gyp:http_parser' ],
} ] ],
'defines': [ 'NODE_EXPERIMENTAL_HTTP_DEFAULT' ],
} ],

[ 'node_shared_http_parser=="false"', {
'dependencies': [
'deps/http_parser/http_parser.gyp:http_parser',
'deps/llhttp/llhttp.gyp:llhttp'
],
} ],

[ 'node_shared_cares=="false"', {
Expand Down
16 changes: 10 additions & 6 deletions src/inspector_socket.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#include "inspector_socket.h"

#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
#define NODE_EXPERIMENTAL_HTTP
#endif
#include "http_parser_adaptor.h"

#include "util-inl.h"

#define NODE_WANT_INTERNALS 1
Expand Down Expand Up @@ -433,13 +437,13 @@ class HttpHandler : public ProtocolHandler {
explicit HttpHandler(InspectorSocket* inspector, TcpHolder::Pointer tcp)
: ProtocolHandler(inspector, std::move(tcp)),
parsing_value_(false) {
#ifdef NODE_EXPERIMENTAL_HTTP
#ifdef NODE_EXPERIMENTAL_HTTP_DEFAULT
llhttp_init(&parser_, HTTP_REQUEST, &parser_settings);
llhttp_settings_init(&parser_settings);
#else /* !NODE_EXPERIMENTAL_HTTP */
#else /* !NODE_EXPERIMENTAL_HTTP_DEFAULT */
http_parser_init(&parser_, HTTP_REQUEST);
http_parser_settings_init(&parser_settings);
#endif /* NODE_EXPERIMENTAL_HTTP */
#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 @@ -484,17 +488,17 @@ class HttpHandler : public ProtocolHandler {

void OnData(std::vector<char>* data) override {
parser_errno_t err;
#ifdef NODE_EXPERIMENTAL_HTTP
#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 */
#else /* !NODE_EXPERIMENTAL_HTTP_DEFAULT */
http_parser_execute(&parser_, &parser_settings, data->data(), data->size());
err = HTTP_PARSER_ERRNO(&parser_);
#endif /* NODE_EXPERIMENTAL_HTTP */
#endif /* NODE_EXPERIMENTAL_HTTP_DEFAULT */
data->clear();
if (err != HPE_OK) {
CancelHandshake();
Expand Down
1 change: 1 addition & 0 deletions src/node_binding.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
V(heap_utils) \
V(http2) \
V(http_parser) \
V(http_parser_llhttp) \
V(inspector) \
V(js_stream) \
V(messaging) \
Expand Down
18 changes: 12 additions & 6 deletions src/node_http_parser.cc → src/node_http_parser_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

// This file is included from 2 files, node_http_parser_traditional.cc
// and node_http_parser_llhttp.cc.

#ifndef SRC_NODE_HTTP_PARSER_IMPL_H_
#define SRC_NODE_HTTP_PARSER_IMPL_H_

#include "node.h"
#include "node_buffer.h"
#include "node_internals.h"
Expand Down Expand Up @@ -47,7 +53,7 @@


namespace node {
namespace {
namespace { // NOLINT(build/namespaces)

using v8::Array;
using v8::Boolean;
Expand Down Expand Up @@ -910,10 +916,10 @@ const parser_settings_t Parser::settings = {
};


void Initialize(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
void InitializeHttpParser(Local<Object> target,
Local<Value> unused,
Local<Context> context,
void* priv) {
Environment* env = Environment::GetCurrent(context);
Local<FunctionTemplate> t = env->NewFunctionTemplate(Parser::New);
t->InstanceTemplate()->SetInternalFieldCount(1);
Expand Down Expand Up @@ -964,4 +970,4 @@ void Initialize(Local<Object> target,
} // anonymous namespace
} // namespace node

NODE_MODULE_CONTEXT_AWARE_INTERNAL(http_parser, node::Initialize)
#endif // SRC_NODE_HTTP_PARSER_IMPL_H_
17 changes: 17 additions & 0 deletions src/node_http_parser_llhttp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#define NODE_EXPERIMENTAL_HTTP 1
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved

#include "node_http_parser_impl.h"

namespace node {

const char* llhttp_version =
NODE_STRINGIFY(LLHTTP_VERSION_MAJOR)
"."
NODE_STRINGIFY(LLHTTP_VERSION_MINOR)
"."
NODE_STRINGIFY(LLHTTP_VERSION_PATCH);

} // namespace node

NODE_MODULE_CONTEXT_AWARE_INTERNAL(http_parser_llhttp,
node::InitializeHttpParser)
18 changes: 18 additions & 0 deletions src/node_http_parser_traditional.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#ifdef NODE_EXPERIMENTAL_HTTP
#undef NODE_EXPERIMENTAL_HTTP
#endif

#include "node_http_parser_impl.h"

namespace node {

const char* http_parser_version =
NODE_STRINGIFY(HTTP_PARSER_VERSION_MAJOR)
"."
NODE_STRINGIFY(HTTP_PARSER_VERSION_MINOR)
"."
NODE_STRINGIFY(HTTP_PARSER_VERSION_PATCH);

} // namespace node

NODE_MODULE_CONTEXT_AWARE_INTERNAL(http_parser, node::InitializeHttpParser)
3 changes: 3 additions & 0 deletions src/node_internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,9 @@ static inline const char* errno_string(int errorno) {

extern double prog_start_time;

extern const char* llhttp_version;
extern const char* http_parser_version;

void Abort(const v8::FunctionCallbackInfo<v8::Value>& args);
void Chdir(const v8::FunctionCallbackInfo<v8::Value>& args);
void CPUUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down
16 changes: 2 additions & 14 deletions src/node_metadata.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,6 @@
#include "node_crypto.h"
#endif

#ifdef NODE_EXPERIMENTAL_HTTP
#include "llhttp.h"
#else /* !NODE_EXPERIMENTAL_HTTP */
#include "http_parser.h"
#endif /* NODE_EXPERIMENTAL_HTTP */

namespace node {

namespace per_process {
Expand All @@ -32,14 +26,8 @@ Metadata::Versions::Versions() {
modules = NODE_STRINGIFY(NODE_MODULE_VERSION);
nghttp2 = NGHTTP2_VERSION;
napi = NODE_STRINGIFY(NAPI_VERSION);

#ifdef NODE_EXPERIMENTAL_HTTP
llhttp = NODE_STRINGIFY(LLHTTP_VERSION_MAJOR) "." NODE_STRINGIFY(
LLHTTP_VERSION_MINOR) "." NODE_STRINGIFY(LLHTTP_VERSION_PATCH);
#else /* !NODE_EXPERIMENTAL_HTTP */
http_parser = NODE_STRINGIFY(HTTP_PARSER_VERSION_MAJOR) "." NODE_STRINGIFY(
HTTP_PARSER_VERSION_MINOR) "." NODE_STRINGIFY(HTTP_PARSER_VERSION_PATCH);
#endif /* NODE_EXPERIMENTAL_HTTP */
llhttp = llhttp_version;
http_parser = http_parser_version;

#if HAVE_OPENSSL
openssl = crypto::GetOpenSSLVersion();
Expand Down
11 changes: 3 additions & 8 deletions src/node_metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,9 @@ namespace node {
V(ares) \
V(modules) \
V(nghttp2) \
V(napi)

#ifdef NODE_EXPERIMENTAL_HTTP
#define NODE_VERSIONS_KEY_HTTP(V) V(llhttp)
#else /* !NODE_EXPERIMENTAL_HTTP */
#define NODE_VERSIONS_KEY_HTTP(V) V(http_parser)
#endif /* NODE_EXPERIMENTAL_HTTP */
V(napi) \
V(llhttp) \
V(http_parser) \

#if HAVE_OPENSSL
#define NODE_VERSIONS_KEY_CRYPTO(V) V(openssl)
Expand All @@ -31,7 +27,6 @@ namespace node {

#define NODE_VERSIONS_KEYS(V) \
NODE_VERSIONS_KEYS_BASE(V) \
NODE_VERSIONS_KEY_HTTP(V) \
NODE_VERSIONS_KEY_CRYPTO(V)

class Metadata {
Expand Down
14 changes: 14 additions & 0 deletions src/node_options.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
if (syntax_check_only && has_eval_string) {
errors->push_back("either --check or --eval can be used, not both");
}

if (http_parser != "legacy" && http_parser != "llhttp") {
errors->push_back("invalid value for --http-parser");
}

debug_options->CheckOptions(errors);
}

Expand Down Expand Up @@ -102,6 +107,15 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
&EnvironmentOptions::experimental_worker,
kAllowedInEnvironment);
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",
"(with --experimental-modules) use the specified file as a "
"custom loader",
Expand Down
6 changes: 6 additions & 0 deletions src/node_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ 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
bool no_deprecation = false;
bool no_force_async_hooks_checks = false;
bool no_warnings = false;
Expand Down
Loading