From ece507394a563bd7e3555a840a3d6980ffbb01ac Mon Sep 17 00:00:00 2001 From: Daniel Beckert Date: Thu, 13 Dec 2018 15:35:48 -0200 Subject: [PATCH] src: do not reuse async resource in http parsers Change resource being used, previously HTTParser was being reused. We are now using IncomingMessage and ClientRequest objects. The goal here is to make the async resource unique for each async operatio Refs: https://github.com/nodejs/node/pull/24330 Refs: https://github.com/nodejs/diagnostics/issues/248 Refs: https://github.com/nodejs/node/pull/21313 Co-authored-by: Matheus Marchini PR-URL: https://github.com/nodejs/node/pull/25094 Reviewed-By: Matteo Collina Reviewed-By: Benedikt Meurer Reviewed-By: Anna Henningsen --- doc/api/async_hooks.md | 8 ++-- lib/_http_client.js | 3 +- lib/_http_server.js | 16 +++++++- src/async_wrap-inl.h | 5 +++ src/async_wrap.cc | 8 +++- src/async_wrap.h | 10 ++++- src/node_http_parser_impl.h | 28 ++++++------- test/async-hooks/test-httparser-reuse.js | 39 +++++++++++++++++++ test/async-hooks/test-httpparser.request.js | 4 +- test/async-hooks/test-httpparser.response.js | 4 +- test/parallel/test-http-parser.js | 4 +- test/sequential/test-async-wrap-getasyncid.js | 2 + test/sequential/test-http-regr-gh-2928.js | 3 +- 13 files changed, 102 insertions(+), 32 deletions(-) create mode 100644 test/async-hooks/test-httparser-reuse.js diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index 99b996a1af6bc8..3e28aa21a8c3c3 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -236,10 +236,10 @@ The `type` is a string identifying the type of resource that caused resource's constructor. ```text -FSEVENTWRAP, FSREQCALLBACK, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPPARSER, -JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP, SHUTDOWNWRAP, -SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVERWRAP, TCPWRAP, TTYWRAP, -UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST, +FSEVENTWRAP, FSREQCALLBACK, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPINCOMINGMESSAGE, +HTTPCLIENTREQUEST, JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP, +SHUTDOWNWRAP, SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVERWRAP, TCPWRAP, +TTYWRAP, UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST, RANDOMBYTESREQUEST, TLSWRAP, Microtask, Timeout, Immediate, TickObject ``` diff --git a/lib/_http_client.js b/lib/_http_client.js index e23c4909ddf3d0..895e0bd6009dae 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -632,11 +632,10 @@ function emitFreeNT(socket) { } function tickOnSocket(req, socket) { - const isParserReused = parsers.hasItems(); const parser = parsers.alloc(); req.socket = socket; req.connection = socket; - parser.reinitialize(HTTPParser.RESPONSE, isParserReused); + parser.initialize(HTTPParser.RESPONSE, req); parser.socket = socket; parser.outgoing = req; req.parser = parser; diff --git a/lib/_http_server.js b/lib/_http_server.js index ff29b9d2b60262..9f62872e8ec5e8 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -126,6 +126,12 @@ const STATUS_CODES = { const kOnExecute = HTTPParser.kOnExecute | 0; +class HTTPServerAsyncResource { + constructor(type, socket) { + this.type = type; + this.socket = socket; + } +} function ServerResponse(req) { OutgoingMessage.call(this); @@ -349,9 +355,15 @@ function connectionListenerInternal(server, socket) { socket.setTimeout(server.timeout); socket.on('timeout', socketOnTimeout); - const isParserReused = parsers.hasItems(); const parser = parsers.alloc(); - parser.reinitialize(HTTPParser.REQUEST, isParserReused); + + // TODO(addaleax): This doesn't play well with the + // `async_hooks.currentResource()` proposal, see + // https://github.com/nodejs/node/pull/21313 + parser.initialize( + HTTPParser.REQUEST, + new HTTPServerAsyncResource('HTTPINCOMINGMESSAGE', socket) + ); parser.socket = socket; // We are starting to wait for our headers. diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h index 6ef968933b8c1f..53972493052f5e 100644 --- a/src/async_wrap-inl.h +++ b/src/async_wrap-inl.h @@ -34,6 +34,11 @@ inline AsyncWrap::ProviderType AsyncWrap::provider_type() const { return provider_type_; } +inline AsyncWrap::ProviderType AsyncWrap::set_provider_type( + AsyncWrap::ProviderType provider) { + provider_type_ = provider; + return provider_type_; +} inline double AsyncWrap::get_async_id() const { return async_id_; diff --git a/src/async_wrap.cc b/src/async_wrap.cc index dd84f992273a27..9cfa0924c79568 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -602,11 +602,15 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { env->destroy_async_id_list()->push_back(async_id); } +void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { + AsyncReset(object(), execution_async_id, silent); +} // Generalized call for both the constructor and for handles that are pooled // and reused over their lifetime. This way a new uid can be assigned when // the resource is pulled out of the pool and put back into use. -void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { +void AsyncWrap::AsyncReset(Local resource, double execution_async_id, + bool silent) { if (async_id_ != -1) { // This instance was in use before, we have already emitted an init with // its previous async_id and need to emit a matching destroy for that @@ -643,7 +647,7 @@ void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { if (silent) return; - EmitAsyncInit(env(), object(), + EmitAsyncInit(env(), resource, env()->async_hooks()->provider_string(provider_type()), async_id_, trigger_async_id_); } diff --git a/src/async_wrap.h b/src/async_wrap.h index d84f0c6d2f61db..bcd37bb0c0d5b6 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -46,7 +46,8 @@ namespace node { V(HTTP2STREAM) \ V(HTTP2PING) \ V(HTTP2SETTINGS) \ - V(HTTPPARSER) \ + V(HTTPINCOMINGMESSAGE) \ + V(HTTPCLIENTREQUEST) \ V(JSSTREAM) \ V(MESSAGEPORT) \ V(PIPECONNECTWRAP) \ @@ -147,11 +148,16 @@ class AsyncWrap : public BaseObject { static void DestroyAsyncIdsCallback(Environment* env, void* data); inline ProviderType provider_type() const; + inline ProviderType set_provider_type(ProviderType provider); inline double get_async_id() const; inline double get_trigger_async_id() const; + void AsyncReset(v8::Local resource, + double execution_async_id = -1, + bool silent = false); + void AsyncReset(double execution_async_id = -1, bool silent = false); // Only call these within a valid HandleScope. @@ -202,7 +208,7 @@ class AsyncWrap : public BaseObject { ProviderType provider, double execution_async_id, bool silent); - const ProviderType provider_type_; + ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. double async_id_ = -1; double trigger_async_id_; diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 75ea992dda9748..a8a8db554783fb 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -152,11 +152,13 @@ struct StringPtr { size_t size_; }; - class Parser : public AsyncWrap, public StreamListener { public: Parser(Environment* env, Local wrap, parser_type_t type) - : AsyncWrap(env, wrap, AsyncWrap::PROVIDER_HTTPPARSER), + : AsyncWrap(env, wrap, + type == HTTP_REQUEST ? + AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE : + AsyncWrap::PROVIDER_HTTPCLIENTREQUEST), current_buffer_len_(0), current_buffer_data_(nullptr) { Init(type); @@ -503,12 +505,12 @@ class Parser : public AsyncWrap, public StreamListener { } - static void Reinitialize(const FunctionCallbackInfo& args) { + static void Initialize(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(args[0]->IsInt32()); - CHECK(args[1]->IsBoolean()); - bool isReused = args[1]->IsTrue(); + CHECK(args[1]->IsObject()); + parser_type_t type = static_cast(args[0].As()->Value()); @@ -517,16 +519,16 @@ 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()); - // 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 (isReused) { - parser->AsyncReset(); - } + + AsyncWrap::ProviderType provider = + (type == HTTP_REQUEST ? + AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE + : AsyncWrap::PROVIDER_HTTPCLIENTREQUEST); + + parser->set_provider_type(provider); parser->Init(type); } - template static void Pause(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -958,7 +960,7 @@ void InitializeHttpParser(Local target, env->SetProtoMethod(t, "free", Parser::Free); env->SetProtoMethod(t, "execute", Parser::Execute); env->SetProtoMethod(t, "finish", Parser::Finish); - env->SetProtoMethod(t, "reinitialize", Parser::Reinitialize); + env->SetProtoMethod(t, "initialize", Parser::Initialize); env->SetProtoMethod(t, "pause", Parser::Pause); env->SetProtoMethod(t, "resume", Parser::Pause); env->SetProtoMethod(t, "consume", Parser::Consume); diff --git a/test/async-hooks/test-httparser-reuse.js b/test/async-hooks/test-httparser-reuse.js new file mode 100644 index 00000000000000..b6d82d7d5e9087 --- /dev/null +++ b/test/async-hooks/test-httparser-reuse.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); +const http = require('http'); +const assert = require('assert'); +const { createHook } = require('async_hooks'); +const reused = Symbol('reused'); + +let reusedHTTPParser = false; +const asyncHook = createHook({ + init(asyncId, type, triggerAsyncId, resource) { + if (resource[reused]) { + reusedHTTPParser = true; + } + resource[reused] = true; + } +}); +asyncHook.enable(); + +const server = http.createServer(function(req, res) { + res.end(); +}); + +const PORT = 3000; +const url = 'http://127.0.0.1:' + PORT; + +server.listen(PORT, common.mustCall(() => { + http.get(url, common.mustCall(() => { + server.close(common.mustCall(() => { + server.listen(PORT, common.mustCall(() => { + http.get(url, common.mustCall(() => { + server.close(common.mustCall(() => { + assert.strictEqual(reusedHTTPParser, false); + })); + })); + })); + })); + })); +})); diff --git a/test/async-hooks/test-httpparser.request.js b/test/async-hooks/test-httpparser.request.js index 2e62e6eaa9842b..7ba8feefe34d5d 100644 --- a/test/async-hooks/test-httpparser.request.js +++ b/test/async-hooks/test-httpparser.request.js @@ -21,7 +21,7 @@ const request = Buffer.from( ); const parser = new HTTPParser(REQUEST); -const as = hooks.activitiesOfTypes('HTTPPARSER'); +const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE'); const httpparser = as[0]; assert.strictEqual(as.length, 1); @@ -47,7 +47,7 @@ process.on('exit', onexit); function onexit() { hooks.disable(); - hooks.sanityCheck('HTTPPARSER'); + hooks.sanityCheck('HTTPINCOMINGMESSAGE'); checkInvocations(httpparser, { init: 1, before: 1, after: 1, destroy: 1 }, 'when process exits'); } diff --git a/test/async-hooks/test-httpparser.response.js b/test/async-hooks/test-httpparser.response.js index 5933b61b79fd99..85d6f76525d49b 100644 --- a/test/async-hooks/test-httpparser.response.js +++ b/test/async-hooks/test-httpparser.response.js @@ -26,7 +26,7 @@ const request = Buffer.from( ); const parser = new HTTPParser(RESPONSE); -const as = hooks.activitiesOfTypes('HTTPPARSER'); +const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST'); const httpparser = as[0]; assert.strictEqual(as.length, 1); @@ -58,7 +58,7 @@ process.on('exit', onexit); function onexit() { hooks.disable(); - hooks.sanityCheck('HTTPPARSER'); + hooks.sanityCheck('HTTPCLIENTREQUEST'); checkInvocations(httpparser, { init: 1, before: 2, after: 2, destroy: 1 }, 'when process exits'); } diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 6d664cfc0adb90..078895d49b13aa 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -95,7 +95,7 @@ function expectBody(expected) { throw new Error('hello world'); }; - parser.reinitialize(HTTPParser.REQUEST, false); + parser.initialize(HTTPParser.REQUEST, request); assert.throws( () => { parser.execute(request, 0, request.length); }, @@ -555,7 +555,7 @@ function expectBody(expected) { parser[kOnBody] = expectBody('ping'); parser.execute(req1, 0, req1.length); - parser.reinitialize(REQUEST, false); + parser.initialize(REQUEST, req2); parser[kOnBody] = expectBody('pong'); parser[kOnHeadersComplete] = onHeadersComplete2; parser.execute(req2, 0, req2.length); diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index 0818dec2db9c3f..e3cfd6bef01bc9 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -48,6 +48,8 @@ const { getSystemErrorName } = require('util'); if (!common.isMainThread) delete providers.INSPECTORJSBINDING; delete providers.KEYPAIRGENREQUEST; + delete providers.HTTPCLIENTREQUEST; + delete providers.HTTPINCOMINGMESSAGE; const objKeys = Object.keys(providers); if (objKeys.length > 0) diff --git a/test/sequential/test-http-regr-gh-2928.js b/test/sequential/test-http-regr-gh-2928.js index 91db08df47cb97..400dc02013325d 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 { HTTPParser } = require('_http_common'); +const { AsyncResource } = require('async_hooks'); const net = require('net'); const COUNT = httpCommon.parsers.max + 1; @@ -24,7 +25,7 @@ function execAndClose() { process.stdout.write('.'); const parser = parsers.pop(); - parser.reinitialize(HTTPParser.RESPONSE, !!parser.reused); + parser.initialize(HTTPParser.RESPONSE, new AsyncResource('ClientRequest')); const socket = net.connect(common.PORT); socket.on('error', (e) => {