From 1744205ff565b490f8db72000028b074cce23d5d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 14 Aug 2018 17:01:06 -0700 Subject: [PATCH] http: move process.binding('http_parser') to internalBinding Refs: https://github.com/nodejs/node/issues/22160 PR-URL: https://github.com/nodejs/node/pull/22329 Reviewed-By: Gus Caplan Reviewed-By: Trivikram Kamat Reviewed-By: Jon Moss Reviewed-By: Ruben Bridgewater Reviewed-By: Joyee Cheung Reviewed-By: Anna Henningsen --- benchmark/http/bench-parser.js | 68 ++++++++++--------- lib/_http_client.js | 3 +- lib/_http_common.js | 3 +- lib/_http_server.js | 3 +- lib/internal/bootstrap/node.js | 2 +- lib/internal/child_process.js | 6 +- src/node_http_parser.cc | 2 +- test/async-hooks/test-httpparser.request.js | 17 +++-- test/async-hooks/test-httpparser.response.js | 18 +++-- test/parallel/test-http-parser-bad-ref.js | 5 +- test/parallel/test-http-parser.js | 5 +- ...ocess-binding-internalbinding-whitelist.js | 1 + test/sequential/test-async-wrap-getasyncid.js | 5 +- test/sequential/test-http-regr-gh-2928.js | 4 +- 14 files changed, 84 insertions(+), 58 deletions(-) diff --git a/benchmark/http/bench-parser.js b/benchmark/http/bench-parser.js index d629fe67e59e76..087616f44e71d1 100644 --- a/benchmark/http/bench-parser.js +++ b/benchmark/http/bench-parser.js @@ -1,50 +1,54 @@ 'use strict'; const common = require('../common'); -const HTTPParser = process.binding('http_parser').HTTPParser; -const REQUEST = HTTPParser.REQUEST; -const kOnHeaders = HTTPParser.kOnHeaders | 0; -const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; -const kOnBody = HTTPParser.kOnBody | 0; -const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; -const CRLF = '\r\n'; const bench = common.createBenchmark(main, { len: [4, 8, 16, 32], - n: [1e5], + n: [1e5] +}, { + flags: ['--expose-internals', '--no-warnings'] }); function main({ len, n }) { - var header = `GET /hello HTTP/1.1${CRLF}Content-Type: text/plain${CRLF}`; - - for (var i = 0; i < len; i++) { - header += `X-Filler${i}: ${Math.random().toString(36).substr(2)}${CRLF}`; + const { internalBinding } = require('internal/test/binding'); + const { HTTPParser } = internalBinding('http_parser'); + const REQUEST = HTTPParser.REQUEST; + const kOnHeaders = HTTPParser.kOnHeaders | 0; + const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; + const kOnBody = HTTPParser.kOnBody | 0; + const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; + const CRLF = '\r\n'; + + function processHeader(header, n) { + const parser = newParser(REQUEST); + + bench.start(); + for (var i = 0; i < n; i++) { + parser.execute(header, 0, header.length); + parser.reinitialize(REQUEST); + } + bench.end(n); } - header += CRLF; - processHeader(Buffer.from(header), n); -} + function newParser(type) { + const parser = new HTTPParser(type); -function processHeader(header, n) { - const parser = newParser(REQUEST); + parser.headers = []; - bench.start(); - for (var i = 0; i < n; i++) { - parser.execute(header, 0, header.length); - parser.reinitialize(REQUEST); - } - bench.end(n); -} + parser[kOnHeaders] = function() { }; + parser[kOnHeadersComplete] = function() { }; + parser[kOnBody] = function() { }; + parser[kOnMessageComplete] = function() { }; -function newParser(type) { - const parser = new HTTPParser(type); + return parser; + } - parser.headers = []; + let header = `GET /hello HTTP/1.1${CRLF}Content-Type: text/plain${CRLF}`; - parser[kOnHeaders] = function() { }; - parser[kOnHeadersComplete] = function() { }; - parser[kOnBody] = function() { }; - parser[kOnMessageComplete] = function() { }; + for (var i = 0; i < len; i++) { + header += `X-Filler${i}: ${Math.random().toString(36).substr(2)}${CRLF}`; + } + header += CRLF; - return parser; + processHeader(Buffer.from(header), n); } diff --git a/lib/_http_client.js b/lib/_http_client.js index 7d28cda681d810..372ac0f953ff51 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -24,7 +24,8 @@ const util = require('util'); const net = require('net'); const url = require('url'); -const { HTTPParser } = process.binding('http_parser'); +const { internalBinding } = require('internal/bootstrap/loaders'); +const { HTTPParser } = internalBinding('http_parser'); const assert = require('assert').ok; const { _checkIsHttpToken: checkIsHttpToken, diff --git a/lib/_http_common.js b/lib/_http_common.js index faa6fe629ae416..09f9aebe9b2ff2 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -21,7 +21,8 @@ 'use strict'; -const { methods, HTTPParser } = process.binding('http_parser'); +const { internalBinding } = require('internal/bootstrap/loaders'); +const { methods, HTTPParser } = internalBinding('http_parser'); const FreeList = require('internal/freelist'); const { ondrain } = require('internal/http'); diff --git a/lib/_http_server.js b/lib/_http_server.js index 3d5a1f8f6242f7..6e1b2c90e5351d 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -23,7 +23,8 @@ const util = require('util'); const net = require('net'); -const { HTTPParser } = process.binding('http_parser'); +const { internalBinding } = require('internal/bootstrap/loaders'); +const { HTTPParser } = internalBinding('http_parser'); const assert = require('assert').ok; const { parsers, diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 26ab0c3198cd56..e6c4da25b00d9b 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -344,7 +344,7 @@ // that are whitelisted for access via process.binding()... this is used // to provide a transition path for modules that are being moved over to // internalBinding. - const internalBindingWhitelist = new SafeSet(['uv']); + const internalBindingWhitelist = new SafeSet(['uv', 'http_parser']); process.binding = function binding(name) { return internalBindingWhitelist.has(name) ? internalBinding(name) : diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index c34bc996de9664..d212bbeaa7bca2 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -21,6 +21,8 @@ const dgram = require('dgram'); const util = require('util'); const assert = require('assert'); +const { internalBinding } = require('internal/bootstrap/loaders'); + const { Process } = process.binding('process_wrap'); const { WriteWrap } = process.binding('stream_wrap'); const { Pipe, constants: PipeConstants } = process.binding('pipe_wrap'); @@ -32,12 +34,10 @@ const { owner_symbol } = require('internal/async_hooks').symbols; const { convertToValidSignal } = require('internal/util'); const { isUint8Array } = require('internal/util/types'); const spawn_sync = process.binding('spawn_sync'); -const { HTTPParser } = process.binding('http_parser'); +const { HTTPParser } = internalBinding('http_parser'); const { freeParser } = require('_http_common'); const { kStateSymbol } = require('internal/dgram'); -const { internalBinding } = require('internal/bootstrap/loaders'); - const { UV_EACCES, UV_EAGAIN, diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 318ad0b8e6bf73..bab15a8c4bdf6c 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -775,4 +775,4 @@ void Initialize(Local target, } // anonymous namespace } // namespace node -NODE_BUILTIN_MODULE_CONTEXT_AWARE(http_parser, node::Initialize) +NODE_MODULE_CONTEXT_AWARE_INTERNAL(http_parser, node::Initialize) diff --git a/test/async-hooks/test-httpparser.request.js b/test/async-hooks/test-httpparser.request.js index c6e18be5a63d38..71fccc0a858921 100644 --- a/test/async-hooks/test-httpparser.request.js +++ b/test/async-hooks/test-httpparser.request.js @@ -1,3 +1,4 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); @@ -6,17 +7,21 @@ const tick = require('./tick'); const initHooks = require('./init-hooks'); const { checkInvocations } = require('./hook-checks'); -const binding = process.binding('http_parser'); -const HTTPParser = binding.HTTPParser; +const hooks = initHooks(); +hooks.enable(); + +// The hooks.enable() must come before require('internal/test/binding') +// 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; const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; -const hooks = initHooks(); - -hooks.enable(); - const request = Buffer.from( 'GET /hello HTTP/1.1\r\n\r\n' ); diff --git a/test/async-hooks/test-httpparser.response.js b/test/async-hooks/test-httpparser.response.js index d56e3e1fecae2d..eea40b14d16b68 100644 --- a/test/async-hooks/test-httpparser.response.js +++ b/test/async-hooks/test-httpparser.response.js @@ -1,3 +1,4 @@ +// Flags: --expose-internals 'use strict'; const common = require('../common'); @@ -6,17 +7,22 @@ const tick = require('./tick'); const initHooks = require('./init-hooks'); const { checkInvocations } = require('./hook-checks'); -const binding = process.binding('http_parser'); -const HTTPParser = binding.HTTPParser; +const hooks = initHooks(); + +hooks.enable(); + +// The hooks.enable() must come before require('internal/test/binding') +// 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; const kOnBody = HTTPParser.kOnBody | 0; -const hooks = initHooks(); - -hooks.enable(); - const request = Buffer.from( 'HTTP/1.1 200 OK\r\n' + 'Content-Type: text/plain\r\n' + diff --git a/test/parallel/test-http-parser-bad-ref.js b/test/parallel/test-http-parser-bad-ref.js index 6e9fb2f9ac6033..60151c63488143 100644 --- a/test/parallel/test-http-parser-bad-ref.js +++ b/test/parallel/test-http-parser-bad-ref.js @@ -2,11 +2,12 @@ // Run this program with valgrind or efence with --expose_gc to expose the // problem. -// Flags: --expose_gc +// Flags: --expose_gc --expose-internals require('../common'); const assert = require('assert'); -const HTTPParser = process.binding('http_parser').HTTPParser; +const { internalBinding } = require('internal/test/binding'); +const { HTTPParser } = internalBinding('http_parser'); const kOnHeaders = HTTPParser.kOnHeaders | 0; const kOnHeadersComplete = HTTPParser.kOnHeadersComplete | 0; diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index bc9c6920bc36a2..0bdaa22b8ade5a 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -19,11 +19,14 @@ // 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 { methods, HTTPParser } = process.binding('http_parser'); +const { internalBinding } = require('internal/test/binding'); +const { methods, HTTPParser } = internalBinding('http_parser'); const { REQUEST, RESPONSE } = HTTPParser; const kOnHeaders = HTTPParser.kOnHeaders | 0; diff --git a/test/parallel/test-process-binding-internalbinding-whitelist.js b/test/parallel/test-process-binding-internalbinding-whitelist.js index ece967a0b76ab9..bfb265a2994952 100644 --- a/test/parallel/test-process-binding-internalbinding-whitelist.js +++ b/test/parallel/test-process-binding-internalbinding-whitelist.js @@ -7,3 +7,4 @@ const assert = require('assert'); // Assert that whitelisted internalBinding modules are accessible via // process.binding(). assert(process.binding('uv')); +assert(process.binding('http_parser')); diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index 1f77267922689d..9ee32b646cd955 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -1,7 +1,8 @@ 'use strict'; -// Flags: --expose-gc +// Flags: --expose-gc --expose-internals const common = require('../common'); +const { internalBinding } = require('internal/test/binding'); const assert = require('assert'); const fs = require('fs'); const fsPromises = fs.promises; @@ -146,7 +147,7 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check { - const HTTPParser = process.binding('http_parser').HTTPParser; + const { HTTPParser } = internalBinding('http_parser'); testInitialized(new HTTPParser(), 'HTTPParser'); } diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js index 55e3a93bc98eaa..0950b84bbe093d 100644 --- a/test/sequential/test-http-regr-gh-2928.js +++ b/test/sequential/test-http-regr-gh-2928.js @@ -1,11 +1,13 @@ // This test is designed to fail with a segmentation fault in Node.js 4.1.0 and // execute without issues in Node.js 4.1.1 and up. +// Flags: --expose-internals 'use strict'; const common = require('../common'); const assert = require('assert'); const httpCommon = require('_http_common'); -const HTTPParser = process.binding('http_parser').HTTPParser; +const { internalBinding } = require('internal/test/binding'); +const { HTTPParser } = internalBinding('http_parser'); const net = require('net'); const COUNT = httpCommon.parsers.max + 1;