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>
nodejs · Dec 7, 2018 · c8d5e31 · c8d5e31
1 parent a22ac0b
commit c8d5e31
Show file tree

Hide file tree

Showing 29 changed files with 157 additions and 85 deletions.
diff --git a/configure.py b/configure.py
@@ -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',

diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -119,6 +119,22 @@ 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
+-->
+
+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 exists to aid in experimentation with the internal implementation of
+the Node.js http parser.
+This flag is likely to become a no-op and removed at some point in the future.
+
 ### `--icu-data-dir=file`
 <!-- YAML
 added: v0.11.15

diff --git a/doc/node.1 b/doc/node.1
@@ -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).

diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -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');

diff --git a/lib/_http_common.js b/lib/_http_common.js
@@ -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');
@@ -243,5 +247,6 @@ module.exports = {
   httpSocketSetup,
   methods,
   parsers,
-  kIncomingMessage
+  kIncomingMessage,
+  HTTPParser
 };
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -23,7 +23,6 @@
 
 const util = require('util');
 const net = require('net');
-const { HTTPParser } = internalBinding('http_parser');
 const assert = require('assert').ok;
 const {
   parsers,
@@ -34,6 +33,7 @@ const {
   chunkExpression,
   httpSocketSetup,
   kIncomingMessage,
+  HTTPParser,
   _checkInvalidHeaderChar: checkInvalidHeaderChar
 } = require('_http_common');
 const { OutgoingMessage } = require('_http_outgoing');

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
@@ -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

diff --git a/node.gyp b/node.gyp
@@ -352,7 +352,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',
@@ -423,6 +424,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',

diff --git a/node.gypi b/node.gypi
@@ -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"', {

diff --git a/src/inspector_socket.cc b/src/inspector_socket.cc
@@ -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
@@ -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;
@@ -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();

diff --git a/src/node_binding.cc b/src/node_binding.cc
@@ -36,6 +36,7 @@
   V(heap_utils)                                                                \
   V(http2)                                                                     \
   V(http_parser)                                                               \
+  V(http_parser_llhttp)                                                        \
   V(inspector)                                                                 \
   V(js_stream)                                                                 \
   V(messaging)                                                                 \

diff --git a/src/node_http_parser.cc → src/node_http_parser_impl.h b/src/node_http_parser.cc → src/node_http_parser_impl.h
@@ -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"
@@ -47,7 +53,7 @@
 
 
 namespace node {
-namespace {
+namespace {  // NOLINT(build/namespaces)
 
 using v8::Array;
 using v8::Boolean;
@@ -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);
@@ -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_
diff --git a/src/node_http_parser_llhttp.cc b/src/node_http_parser_llhttp.cc
@@ -0,0 +1,17 @@
+#define NODE_EXPERIMENTAL_HTTP 1
+
+#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)
diff --git a/src/node_http_parser_traditional.cc b/src/node_http_parser_traditional.cc
@@ -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)
diff --git a/src/node_internals.h b/src/node_internals.h
@@ -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);

diff --git a/src/node_metadata.cc b/src/node_metadata.cc
@@ -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 {
@@ -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();

diff --git a/src/node_metadata.h b/src/node_metadata.h
@@ -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)
@@ -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 {

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -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);
 }
 
@@ -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",

diff --git a/src/node_options.h b/src/node_options.h
@@ -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;