[v12.x] http: runtime deprecate legacy HTTP parser

[BethGriggs]

The legacy HTTP parser, used by default in versions of Node.js prior to
12.0.0, is deprecated. The legacy HTTP parser cannot be guaranteed to be
supported after April 2021. This commit introduces a deprecation warning
for the legacy HTTP parser.

Refs: #31441

---

I wasn't sure whether the warning should be emitted upon load or when --http-parser=legacy is specified as an option (similar to https://github.com/nodejs/node/blob/master/src/node_options.cc#L41). Happy to move the deprecation to there, if that is preferred.

[richardlau]

Another way would be to emit the warning from the C++ layer which would emit the deprecation even if someone did process.binding('http_parser') regardless of the setting of --http-parser.

diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 7c39bc15c7..6aaba922b5 100644 --- a/src/node_http_parser_impl.h+++ b/src/node_http_parser_impl.h@@ -26,6 +26,9 @@ #include "node.h" #include "node_buffer.h" +#ifndef NODE_EXPERIMENTAL_HTTP+#include "node_process.h"+#endif /* NODE_EXPERIMENTAL_HTTP */ #include "util.h" #include "async_wrap-inl.h" @@ -1021,6 +1024,10 @@ void InitializeHttpParser(Local<Object> target, #ifndef NODE_EXPERIMENTAL_HTTP static uv_once_t init_once = UV_ONCE_INIT; uv_once(&init_once, InitMaxHttpHeaderSizeOnce); + ProcessEmitDeprecationWarning(+ env,+ "The legacy HTTP parser deprecated.",+ "DEP0131").IsNothing(); #endif /* NODE_EXPERIMENTAL_HTTP */ }

[BethGriggs]

@richardlau I think that approach makes sense, changed to that.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36479/

[richardlau]

We may also want to update the documentation for the --http-parser= option? https://nodejs.org/docs/latest-v12.x/api/cli.html#cli_http_parser_library

[mcollina]

LGTM

[mcollina]

I would recommend a few more reviews by the @nodejs/tsc team.

Also, cc @nodejs/http

[jasnell]

Adding a runtime deprecation is typically semver-major and not something we would typically backport, especially not into an LTS line. In this case, there's likely a good reason for us to go ahead and do it given the complete switch over to llhttp. This should at least be a semver-minor.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/36532/

[BethGriggs]

Landed in a0b6104 (v12.x-staging)