From d46dc7958bd477c38190cb796a4ffda434bb0aa4 Mon Sep 17 00:00:00 2001 From: Bastian Krol Date: Thu, 4 Oct 2018 09:30:47 +0200 Subject: [PATCH 1/2] http: remove duplicate async_hooks init for http parser Each time a new parser was created, AsyncReset was being called via the C++ Parser class constructor (super constructor AsyncWrap) and also via Parser::Reinitialize. This also adds a missing async_hooks destroy event before AsyncReset is called for the parser reuse case, otherwise the old async_id never gets destroyed. Refs: #19859 --- benchmark/http/bench-parser.js | 2 +- lib/_http_client.js | 11 +++- lib/_http_server.js | 7 ++- lib/internal/freelist.js | 14 ++++- src/node_http_parser.cc | 10 ++- test/async-hooks/test-graph.http.js | 10 +-- .../test-async-hooks-http-parser-destroy.js | 61 +++++++++++++++++++ test/parallel/test-freelist.js | 23 ++++--- test/parallel/test-http-parser.js | 4 +- test/sequential/test-http-regr-gh-2928.js | 2 +- 10 files changed, 113 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-async-hooks-http-parser-destroy.js diff --git a/benchmark/http/bench-parser.js b/benchmark/http/bench-parser.js index 087616f44e71d1..8208df11223b90 100644 --- a/benchmark/http/bench-parser.js +++ b/benchmark/http/bench-parser.js @@ -25,7 +25,7 @@ function main({ len, n }) { bench.start(); for (var i = 0; i < n; i++) { parser.execute(header, 0, header.length); - parser.reinitialize(REQUEST); + parser.reinitialize(REQUEST, i > 0); } bench.end(n); } diff --git a/lib/_http_client.js b/lib/_http_client.js index c83800a93b1da9..5f759dab09fe36 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -36,7 +36,11 @@ const { const { OutgoingMessage } = require('_http_outgoing'); const Agent = require('_http_agent'); const { Buffer } = require('buffer'); -const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); +const { + defaultTriggerAsyncIdScope, + destroyHooksExist, + emitDestroy +} = require('internal/async_hooks'); const { URL, urlToOptions, searchParamsSymbol } = require('internal/url'); const { outHeadersKey, ondrain } = require('internal/http'); const { @@ -631,7 +635,10 @@ function tickOnSocket(req, socket) { var parser = parsers.alloc(); req.socket = socket; req.connection = socket; - parser.reinitialize(HTTPParser.RESPONSE); + if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) { + emitDestroy(parser.getAsyncId()); + } + parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset); parser.socket = socket; parser.outgoing = req; req.parser = parser; diff --git a/lib/_http_server.js b/lib/_http_server.js index cc1a428cd66c3b..81bc23332a1df1 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -40,6 +40,8 @@ const { OutgoingMessage } = require('_http_outgoing'); const { outHeadersKey, ondrain } = require('internal/http'); const { defaultTriggerAsyncIdScope, + destroyHooksExist, + emitDestroy, getOrSetAsyncId } = require('internal/async_hooks'); const { IncomingMessage } = require('_http_incoming'); @@ -338,7 +340,10 @@ function connectionListenerInternal(server, socket) { socket.on('timeout', socketOnTimeout); var parser = parsers.alloc(); - parser.reinitialize(HTTPParser.REQUEST); + if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) { + emitDestroy(parser.getAsyncId()); + } + parser.reinitialize(HTTPParser.REQUEST, parser.needsAsyncReset); parser.socket = socket; socket.parser = parser; diff --git a/lib/internal/freelist.js b/lib/internal/freelist.js index 7e9cef9528ab75..94e148dc5975f6 100644 --- a/lib/internal/freelist.js +++ b/lib/internal/freelist.js @@ -10,8 +10,8 @@ class FreeList { alloc() { return this.list.length ? - this.list.pop() : - this.ctor.apply(this, arguments); + needsToCallAsyncReset(this.list.pop()) : + mustNotCallAsyncReset(this.ctor.apply(this, arguments)); } free(obj) { @@ -23,4 +23,14 @@ class FreeList { } } +function needsToCallAsyncReset(item) { + item.needsAsyncReset = true; + return item; +} + +function mustNotCallAsyncReset(item) { + item.needsAsyncReset = false; + return item; +} + module.exports = FreeList; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index 5d093b27c39d3c..e05af5279f130e 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -465,6 +465,8 @@ class Parser : public AsyncWrap, public StreamListener { Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsInt32()); + CHECK(args[1]->IsBoolean()); + bool needsAsyncReset = args[1]->IsTrue(); http_parser_type type = static_cast(args[0].As()->Value()); @@ -473,8 +475,12 @@ class Parser : public AsyncWrap, public StreamListener { ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); // Should always be called from the same context. CHECK_EQ(env, parser->env()); - // The parser is being reused. Reset the async id and call init() callbacks. - parser->AsyncReset(); + // This parser has either just been created or it is being reused. + // We must only call AsyncReset for the latter case, because AsyncReset has + // already been called via the constructor for the former case. + if (needsAsyncReset) { + parser->AsyncReset(); + } parser->Init(type); } diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index b18bc7453c0fa2..414ebabeeeaea4 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -38,20 +38,14 @@ process.on('exit', function() { { type: 'HTTPPARSER', id: 'httpparser:1', triggerAsyncId: 'tcpserver:1' }, - { type: 'HTTPPARSER', - id: 'httpparser:2', - triggerAsyncId: 'tcpserver:1' }, { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' }, { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' }, { type: 'HTTPPARSER', - id: 'httpparser:3', - triggerAsyncId: 'tcp:2' }, - { type: 'HTTPPARSER', - id: 'httpparser:4', + id: 'httpparser:2', triggerAsyncId: 'tcp:2' }, { type: 'Timeout', id: 'timeout:2', - triggerAsyncId: 'httpparser:4' }, + triggerAsyncId: 'httpparser:2' }, { type: 'SHUTDOWNWRAP', id: 'shutdown:1', triggerAsyncId: 'tcp:2' } ] diff --git a/test/parallel/test-async-hooks-http-parser-destroy.js b/test/parallel/test-async-hooks-http-parser-destroy.js new file mode 100644 index 00000000000000..aeb805702d89e9 --- /dev/null +++ b/test/parallel/test-async-hooks-http-parser-destroy.js @@ -0,0 +1,61 @@ +'use strict'; +const common = require('../common'); +const Countdown = require('../common/countdown'); +const assert = require('assert'); +const async_hooks = require('async_hooks'); +const http = require('http'); + +// Regression test for https://github.com/nodejs/node/issues/19859. +// Checks that matching destroys are emitted when creating new/reusing old http +// parser instances. + +const N = 50; +const KEEP_ALIVE = 100; + +const createdIds = []; +const destroyedIds = []; +async_hooks.createHook({ + init: common.mustCallAtLeast((asyncId, type) => { + if (type === 'HTTPPARSER') { + createdIds.push(asyncId); + } + }, N), + destroy: (asyncId) => { + destroyedIds.push(asyncId); + } +}).enable(); + +const server = http.createServer(function(req, res) { + res.end('Hello'); +}); + +const keepAliveAgent = new http.Agent({ + keepAlive: true, + keepAliveMsecs: KEEP_ALIVE, +}); + +const countdown = new Countdown(N, () => { + server.close(() => { + // give the server sockets time to close (which will also free their + // associated parser objects) after the server has been closed. + setTimeout(() => { + createdIds.forEach((createdAsyncId) => { + assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0); + }); + }, KEEP_ALIVE * 2); + }); +}); + +server.listen(0, function() { + for (let i = 0; i < N; ++i) { + (function makeRequest() { + http.get({ + port: server.address().port, + agent: keepAliveAgent + }, function(res) { + countdown.dec(); + res.resume(); + }); + })(); + } +}); diff --git a/test/parallel/test-freelist.js b/test/parallel/test-freelist.js index d1f7d888c03868..eb43308dbe59cc 100644 --- a/test/parallel/test-freelist.js +++ b/test/parallel/test-freelist.js @@ -8,24 +8,23 @@ const FreeList = require('internal/freelist'); assert.strictEqual(typeof FreeList, 'function'); -const flist1 = new FreeList('flist1', 3, String); +const flist1 = new FreeList('flist1', 3, Object); // Allocating when empty, should not change the list size -const result = flist1.alloc('test'); -assert.strictEqual(typeof result, 'string'); -assert.strictEqual(result, 'test'); +const result = flist1.alloc(); +assert.strictEqual(typeof result, 'object'); assert.strictEqual(flist1.list.length, 0); // Exhaust the free list -assert(flist1.free('test1')); -assert(flist1.free('test2')); -assert(flist1.free('test3')); +assert(flist1.free({ id: 'test1' })); +assert(flist1.free({ id: 'test2' })); +assert(flist1.free({ id: 'test3' })); // Now it should not return 'true', as max length is exceeded -assert.strictEqual(flist1.free('test4'), false); -assert.strictEqual(flist1.free('test5'), false); +assert.strictEqual(flist1.free({ id: 'test4' }), false); +assert.strictEqual(flist1.free({ id: 'test5' }), false); // At this point 'alloc' should just return the stored values -assert.strictEqual(flist1.alloc(), 'test3'); -assert.strictEqual(flist1.alloc(), 'test2'); -assert.strictEqual(flist1.alloc(), 'test1'); +assert.strictEqual(flist1.alloc().id, 'test3'); +assert.strictEqual(flist1.alloc().id, 'test2'); +assert.strictEqual(flist1.alloc().id, 'test1'); diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 0bdaa22b8ade5a..36f41f79e59dcf 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -98,7 +98,7 @@ function expectBody(expected) { throw new Error('hello world'); }; - parser.reinitialize(HTTPParser.REQUEST); + parser.reinitialize(HTTPParser.REQUEST, false); assert.throws( () => { parser.execute(request, 0, request.length); }, @@ -558,7 +558,7 @@ function expectBody(expected) { parser[kOnBody] = expectBody('ping'); parser.execute(req1, 0, req1.length); - parser.reinitialize(REQUEST); + parser.reinitialize(REQUEST, false); parser[kOnBody] = expectBody('pong'); parser[kOnHeadersComplete] = onHeadersComplete2; parser.execute(req2, 0, req2.length); diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js index 0950b84bbe093d..a686fa6c522d61 100644 --- a/test/sequential/test-http-regr-gh-2928.js +++ b/test/sequential/test-http-regr-gh-2928.js @@ -25,7 +25,7 @@ function execAndClose() { process.stdout.write('.'); const parser = parsers.pop(); - parser.reinitialize(HTTPParser.RESPONSE); + parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset); const socket = net.connect(common.PORT); socket.on('error', (e) => { From 26d147113f220dc3c65d9b19412dcc9dc1fe62d6 Mon Sep 17 00:00:00 2001 From: Bastian Krol Date: Thu, 4 Oct 2018 22:09:40 +0200 Subject: [PATCH 2/2] http: use symbol for FreeList's is_reused This also at least keeps up the appearance of FreeList being a generic all purpose thing (right now it is only used for HTTP parsers but theoretically it could be used for any reusable resource). The previous name "needsAsyncReset" implied a tight coupling to to async resources, the new name "is_reused" is more generic. --- benchmark/misc/freelist.js | 2 +- lib/_http_client.js | 5 +++-- lib/_http_common.js | 2 +- lib/_http_server.js | 5 +++-- lib/internal/freelist.js | 22 ++++++++++--------- src/node_http_parser.cc | 4 ++-- test/parallel/test-freelist.js | 2 +- test/parallel/test-internal-modules-expose.js | 2 +- test/sequential/test-http-regr-gh-2928.js | 3 ++- 9 files changed, 26 insertions(+), 21 deletions(-) diff --git a/benchmark/misc/freelist.js b/benchmark/misc/freelist.js index 8c3281cc407363..7fa9af4f3ddb7f 100644 --- a/benchmark/misc/freelist.js +++ b/benchmark/misc/freelist.js @@ -9,7 +9,7 @@ const bench = common.createBenchmark(main, { }); function main({ n }) { - const FreeList = require('internal/freelist'); + const { FreeList } = require('internal/freelist'); const poolSize = 1000; const list = new FreeList('test', poolSize, Object); var j; diff --git a/lib/_http_client.js b/lib/_http_client.js index 5f759dab09fe36..ccfaea03d56c5e 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -51,6 +51,7 @@ const { ERR_UNESCAPED_CHARACTERS } = require('internal/errors').codes; const { validateTimerDuration } = require('internal/timers'); +const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol; const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; @@ -635,10 +636,10 @@ function tickOnSocket(req, socket) { var parser = parsers.alloc(); req.socket = socket; req.connection = socket; - if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) { + if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) { emitDestroy(parser.getAsyncId()); } - parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset); + parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]); parser.socket = socket; parser.outgoing = req; req.parser = parser; diff --git a/lib/_http_common.js b/lib/_http_common.js index 1de0ee6025d571..b37814f7832242 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -23,7 +23,7 @@ const { methods, HTTPParser } = internalBinding('http_parser'); -const FreeList = require('internal/freelist'); +const { FreeList } = require('internal/freelist'); const { ondrain } = require('internal/http'); const incoming = require('_http_incoming'); const { diff --git a/lib/_http_server.js b/lib/_http_server.js index 81bc23332a1df1..b45ce78c4ed63c 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -44,6 +44,7 @@ const { emitDestroy, getOrSetAsyncId } = require('internal/async_hooks'); +const is_reused_symbol = require('internal/freelist').symbols.is_reused_symbol; const { IncomingMessage } = require('_http_incoming'); const { ERR_HTTP_HEADERS_SENT, @@ -340,10 +341,10 @@ function connectionListenerInternal(server, socket) { socket.on('timeout', socketOnTimeout); var parser = parsers.alloc(); - if (destroyHooksExist() && parser.needsAsyncReset && parser.getAsyncId()) { + if (destroyHooksExist() && parser[is_reused_symbol] && parser.getAsyncId()) { emitDestroy(parser.getAsyncId()); } - parser.reinitialize(HTTPParser.REQUEST, parser.needsAsyncReset); + parser.reinitialize(HTTPParser.REQUEST, parser[is_reused_symbol]); parser.socket = socket; socket.parser = parser; diff --git a/lib/internal/freelist.js b/lib/internal/freelist.js index 94e148dc5975f6..bb7e35300659c3 100644 --- a/lib/internal/freelist.js +++ b/lib/internal/freelist.js @@ -1,5 +1,7 @@ 'use strict'; +const is_reused_symbol = Symbol('isReused'); + class FreeList { constructor(name, max, ctor) { this.name = name; @@ -10,8 +12,8 @@ class FreeList { alloc() { return this.list.length ? - needsToCallAsyncReset(this.list.pop()) : - mustNotCallAsyncReset(this.ctor.apply(this, arguments)); + setIsReused(this.list.pop(), true) : + setIsReused(this.ctor.apply(this, arguments), false); } free(obj) { @@ -23,14 +25,14 @@ class FreeList { } } -function needsToCallAsyncReset(item) { - item.needsAsyncReset = true; +function setIsReused(item, reused) { + item[is_reused_symbol] = reused; return item; } -function mustNotCallAsyncReset(item) { - item.needsAsyncReset = false; - return item; -} - -module.exports = FreeList; +module.exports = { + FreeList, + symbols: { + is_reused_symbol + } +}; diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc index e05af5279f130e..9850b4f698205b 100644 --- a/src/node_http_parser.cc +++ b/src/node_http_parser.cc @@ -466,7 +466,7 @@ class Parser : public AsyncWrap, public StreamListener { CHECK(args[0]->IsInt32()); CHECK(args[1]->IsBoolean()); - bool needsAsyncReset = args[1]->IsTrue(); + bool isReused = args[1]->IsTrue(); http_parser_type type = static_cast(args[0].As()->Value()); @@ -478,7 +478,7 @@ class Parser : public AsyncWrap, public StreamListener { // This parser has either just been created or it is being reused. // We must only call AsyncReset for the latter case, because AsyncReset has // already been called via the constructor for the former case. - if (needsAsyncReset) { + if (isReused) { parser->AsyncReset(); } parser->Init(type); diff --git a/test/parallel/test-freelist.js b/test/parallel/test-freelist.js index eb43308dbe59cc..03946dfda257c2 100644 --- a/test/parallel/test-freelist.js +++ b/test/parallel/test-freelist.js @@ -4,7 +4,7 @@ require('../common'); const assert = require('assert'); -const FreeList = require('internal/freelist'); +const { FreeList } = require('internal/freelist'); assert.strictEqual(typeof FreeList, 'function'); diff --git a/test/parallel/test-internal-modules-expose.js b/test/parallel/test-internal-modules-expose.js index a3fd6f63ffe399..ab48e36881268c 100644 --- a/test/parallel/test-internal-modules-expose.js +++ b/test/parallel/test-internal-modules-expose.js @@ -7,5 +7,5 @@ const config = process.binding('config'); console.log(config, process.argv); -assert.strictEqual(typeof require('internal/freelist'), 'function'); +assert.strictEqual(typeof require('internal/freelist').FreeList, 'function'); assert.strictEqual(config.exposeInternals, true); diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js index a686fa6c522d61..3794eddaa09369 100644 --- a/test/sequential/test-http-regr-gh-2928.js +++ b/test/sequential/test-http-regr-gh-2928.js @@ -7,6 +7,7 @@ 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 net = require('net'); @@ -25,7 +26,7 @@ function execAndClose() { process.stdout.write('.'); const parser = parsers.pop(); - parser.reinitialize(HTTPParser.RESPONSE, parser.needsAsyncReset); + parser.reinitialize(HTTPParser.RESPONSE, parser[is_reused_symbol]); const socket = net.connect(common.PORT); socket.on('error', (e) => {