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: https://github.com/nodejs/node/pull/24739
Refs: https://github.com/nodejs/node/issues/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>

configure.py

@@ -187,7 +187,7 @@ parser.add_option('--openssl-system-ca-path',
 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',

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

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

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');

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

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');

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

node.gyp

@@ -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',
@@ -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',

node.gypi

@@ -164,12 +164,14 @@
     }],
 
     [ 'node_experimental_http_parser=="true"', {
-      'defines': [ 'NODE_EXPERIMENTAL_HTTP' ],
+      'defines': [ 'NODE_EXPERIMENTAL_HTTP_DEFAULT' ],
-      'dependencies': [ 'deps/llhttp/llhttp.gyp:llhttp' ],
+    } ],
-    }, {
+
-      'conditions': [ [ 'node_shared_http_parser=="false"', {
+    [ 'node_shared_http_parser=="false"', {
-        'dependencies': [ 'deps/http_parser/http_parser.gyp:http_parser' ],
+      'dependencies': [
-      } ] ],
+        'deps/http_parser/http_parser.gyp:http_parser',
+        'deps/llhttp/llhttp.gyp:llhttp'
+      ],
     } ],
 
     [ 'node_shared_cares=="false"', {

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();

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

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,
+void InitializeHttpParser(Local<Object> target,
-                Local<Value> unused,
+                          Local<Value> unused,
-                Local<Context> context,
+                          Local<Context> context,
-                void* priv) {
+                          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_

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)

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)

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

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);
-
+  llhttp = llhttp_version;
-#ifdef NODE_EXPERIMENTAL_HTTP
+  http_parser = http_parser_version;
-  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 */
 
 #if HAVE_OPENSSL
   openssl = crypto::GetOpenSSLVersion();

src/node_metadata.h

@@ -15,13 +15,9 @@ namespace node {
   V(ares)                                                                      \
   V(modules)                                                                   \
   V(nghttp2)                                                                   \
-  V(napi)
+  V(napi)                                                                      \
-
+  V(llhttp)                                                                    \
-#ifdef NODE_EXPERIMENTAL_HTTP
+  V(http_parser)                                                               \
-#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 */
 
 #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 {

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

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;

test/async-hooks/test-httpparser.request.js

@@ -10,13 +10,7 @@ const { checkInvocations } = require('./hook-checks');
 const hooks = initHooks();
 hooks.enable();
 
-// The hooks.enable() must come before require('internal/test/binding')
+const { HTTPParser } = require('_http_common');
-// because internal/test/binding schedules a process warning on nextTick.
-// If this order is not preserved, the hooks check will fail because it
-// will not be notified about the nextTick creation but will see the
-// callback event.
-const { internalBinding } = require('internal/test/binding');
-const { HTTPParser } = internalBinding('http_parser');
 
 const REQUEST = HTTPParser.REQUEST;
 

test/async-hooks/test-httpparser.response.js

@@ -11,13 +11,7 @@ const hooks = initHooks();
 
 hooks.enable();
 
-// The hooks.enable() must come before require('internal/test/binding')
+const { HTTPParser } = require('_http_common');
-// because internal/test/binding schedules a process warning on nextTick.
-// If this order is not preserved, the hooks check will fail because it
-// will not be notified about the nextTick creation but will see the
-// callback event.
-const { internalBinding } = require('internal/test/binding');
-const { HTTPParser } = internalBinding('http_parser');
 
 const RESPONSE = HTTPParser.RESPONSE;
 const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;

test/parallel/test-http-parser-bad-ref.js

@@ -2,12 +2,11 @@
 // Run this program with valgrind or efence with --expose_gc to expose the
 // problem.
 
-// Flags: --expose_gc --expose-internals
+// Flags: --expose_gc
 
 require('../common');
 const assert = require('assert');
-const { internalBinding } = require('internal/test/binding');
+const { HTTPParser } = require('_http_common');
-const { HTTPParser } = internalBinding('http_parser');
 
 const kOnHeaders = HTTPParser.kOnHeaders | 0;
 const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0;

test/parallel/test-http-parser-lazy-loaded.js

@@ -3,6 +3,7 @@
 'use strict';
 
 const { internalBinding } = require('internal/test/binding');
+const { getOptionValue } = require('internal/options');
 
 // Monkey patch before requiring anything
 class DummyParser {
@@ -11,7 +12,11 @@ class DummyParser {
   }
 }
 DummyParser.REQUEST = Symbol();
-internalBinding('http_parser').HTTPParser = DummyParser;
+
+const binding =
+  getOptionValue('--http-parser') === 'legacy' ?
+    internalBinding('http_parser') : internalBinding('http_parser_llhttp');
+binding.HTTPParser = DummyParser;
 
 const common = require('../common');
 const assert = require('assert');

test/parallel/test-http-parser.js

@@ -19,14 +19,11 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-// Flags: --expose-internals
-
 'use strict';
 const { mustCall, mustNotCall } = require('../common');
 const assert = require('assert');
 
-const { internalBinding } = require('internal/test/binding');
+const { methods, HTTPParser } = require('_http_common');
-const { methods, HTTPParser } = internalBinding('http_parser');
 const { REQUEST, RESPONSE } = HTTPParser;
 
 const kOnHeaders = HTTPParser.kOnHeaders | 0;

test/parallel/test-process-versions.js

@@ -3,7 +3,8 @@ const common = require('../common');
 const assert = require('assert');
 
 const expected_keys = ['ares', 'modules', 'node',
-                       'uv', 'v8', 'zlib', 'nghttp2', 'napi'];
+                       'uv', 'v8', 'zlib', 'nghttp2', 'napi',
+                       'http_parser', 'llhttp'];
 
 if (common.hasCrypto) {
   expected_keys.push('openssl');
@@ -16,9 +17,6 @@ if (common.hasIntl) {
   expected_keys.push('unicode');
 }
 
-expected_keys.push(
-  process.versions.llhttp === undefined ? 'http_parser' : 'llhttp');
-
 expected_keys.sort();
 const actual_keys = Object.keys(process.versions).sort();
 
@@ -27,8 +25,8 @@ assert.deepStrictEqual(actual_keys, expected_keys);
 const commonTemplate = /^\d+\.\d+\.\d+(?:-.*)?$/;
 
 assert(commonTemplate.test(process.versions.ares));
-assert(commonTemplate.test(process.versions.llhttp === undefined ?
+assert(commonTemplate.test(process.versions.llhttp));
-  process.versions.http_parser : process.versions.llhttp));
+assert(commonTemplate.test(process.versions.http_parser));
 assert(commonTemplate.test(process.versions.node));
 assert(commonTemplate.test(process.versions.uv));
 assert(commonTemplate.test(process.versions.zlib));

test/parallel/test-trace-events-metadata.js

@@ -36,9 +36,10 @@ proc.once('exit', common.mustCall(() => {
 
     assert(traces.some((trace) =>
       trace.name === 'node' &&
-        (trace.args.process.versions.http_parser ===
+        trace.args.process.versions.http_parser ===
-           process.versions.http_parser ||
+          process.versions.http_parser &&
-         trace.args.process.versions.llhttp === process.versions.llhttp) &&
+        trace.args.process.versions.llhttp ===
+          process.versions.llhttp &&
         trace.args.process.versions.node ===
           process.versions.node &&
         trace.args.process.versions.v8 ===

test/sequential/test-async-wrap-getasyncid.js

@@ -148,7 +148,7 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check
 
 
 {
-  const { HTTPParser } = internalBinding('http_parser');
+  const { HTTPParser } = require('_http_common');
   testInitialized(new HTTPParser(HTTPParser.REQUEST), 'HTTPParser');
 }
 

test/sequential/test-http-max-http-headers.js

@@ -1,11 +1,13 @@
+// Flags: --expose-internals
 'use strict';
-
-const assert = require('assert');
 const common = require('../common');
+const assert = require('assert');
 const http = require('http');
 const net = require('net');
 const MAX = 8 * 1024; // 8KB
 
+const { getOptionValue } = require('internal/options');
+
 // Verify that we cannot receive more than 8KB of headers.
 
 function once(cb) {
@@ -27,7 +29,7 @@ function finished(client, callback) {
 function fillHeaders(headers, currentSize, valid = false) {
   // llhttp counts actual header name/value sizes, excluding the whitespace and
   // stripped chars.
-  if (process.versions.hasOwnProperty('llhttp')) {
+  if (getOptionValue('--http-parser') === 'llhttp') {
     // OK, Content-Length, 0, X-CRASH, aaa...
     headers += 'a'.repeat(MAX - currentSize);
   } else {

test/sequential/test-http-regr-gh-2928.js

@@ -6,9 +6,8 @@
 const common = require('../common');
 const assert = require('assert');
 const httpCommon = require('_http_common');
-const { internalBinding } = require('internal/test/binding');
 const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol;
-const { HTTPParser } = internalBinding('http_parser');
+const { HTTPParser } = require('_http_common');
 const net = require('net');
 
 const COUNT = httpCommon.parsers.max + 1;