From 60479d4bcbe988aedd4cde7090bad5fcc6d68f57 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Tue, 23 Apr 2019 00:57:12 +0200 Subject: [PATCH 1/7] async_hooks: fixup do not reuse HTTPParser Fix some issues introduced/not fixed via https://github.com/nodejs/node/pull/25094: * Init hook is not emitted for a reused HTTPParser * HTTPParser was still used as resource in init hook * type used in init hook was always HTTPINCOMINGMESSAGE even for client requests * some tests have not been adapted to new resource names With this change the async hooks init event is emitted during a call to Initialize() as the type and resource object is available at this time. As a result Initialize() must be called now which could be seen as breaking change even HTTPParser is not part of documented API. It was needed to put the ClientRequest instance into a wrapper object instead passing it directly as async resource otherwise test-domain-multi fails. I think this is because adding an EventEmitter to a Domain adds a property 'domain' and the presence of this changes the context propagation in domains. Besides that tests still refering to resource HTTPParser have been updated/improved. Fixes: https://github.com/nodejs/node/issues/27467 Fixes: https://github.com/nodejs/node/issues/26961 Refs: https://github.com/nodejs/node/pull/25094 --- lib/_http_client.js | 10 +- src/async_wrap.cc | 125 +++++++++--------- src/async_wrap.h | 118 ++++++++--------- src/node_http_parser_impl.h | 7 +- test/async-hooks/coverage.md | 3 +- test/async-hooks/test-graph.http.js | 8 +- test/async-hooks/test-graph.tls-write.js | 4 - test/async-hooks/test-httparser-reuse.js | 55 ++++++-- test/async-hooks/test-httpparser.request.js | 1 + test/async-hooks/test-httpparser.response.js | 1 + test/async-hooks/verify-graph.js | 12 ++ .../test-async-hooks-http-parser-destroy.js | 5 +- 12 files changed, 203 insertions(+), 146 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 895e0bd6009dae..75c86acf2c510c 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -64,6 +64,13 @@ function validateHost(host, name) { return host; } +class HTTPClientAsyncResource { + constructor(type, req) { + this.type = type; + this.req = req; + } +} + let urlWarningEmitted = false; function ClientRequest(input, options, cb) { OutgoingMessage.call(this); @@ -635,7 +642,8 @@ function tickOnSocket(req, socket) { const parser = parsers.alloc(); req.socket = socket; req.connection = socket; - parser.initialize(HTTPParser.RESPONSE, req); + parser.initialize(HTTPParser.RESPONSE, + new HTTPClientAsyncResource('HTTPINCOMINGMESSAGE', req)); parser.socket = socket; parser.outgoing = req; req.parser = parser; diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 9cfa0924c79568..f7fa7a5c61b2e7 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -63,7 +63,7 @@ namespace node { static const char* const provider_names[] = { #define V(PROVIDER) \ #PROVIDER, - NODE_ASYNC_PROVIDER_TYPES(V) + NODE_ASYNC_PROVIDER_TYPES(V) #undef V }; @@ -131,12 +131,12 @@ void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) { void AsyncWrap::EmitTraceEventBefore() { switch (provider_type()) { -#define V(PROVIDER) \ - case PROVIDER_ ## PROVIDER: \ +#define V(PROVIDER) \ + case PROVIDER_##PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_BEGIN0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER "_CALLBACK", static_cast(get_async_id())); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -153,12 +153,12 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) { void AsyncWrap::EmitTraceEventAfter(ProviderType type, double async_id) { switch (type) { -#define V(PROVIDER) \ - case PROVIDER_ ## PROVIDER: \ +#define V(PROVIDER) \ + case PROVIDER_##PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_END0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER "_CALLBACK", static_cast(async_id)); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -177,7 +177,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { class PromiseWrap : public AsyncWrap { public: PromiseWrap(Environment* env, Local object, bool silent) - : AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) { + : AsyncWrap(env, object, PROVIDER_PROMISE, invalid_async_id, silent) { MakeWeak(); } @@ -214,7 +214,7 @@ PromiseWrap* PromiseWrap::New(Environment* env, void PromiseWrap::getIsChainedPromise(Local property, const PropertyCallbackInfo& info) { info.GetReturnValue().Set( - info.Holder()->GetInternalField(kIsChainedPromiseField)); + info.Holder()->GetInternalField(kIsChainedPromiseField)); } static PromiseWrap* extractPromiseWrap(Local promise) { @@ -293,11 +293,11 @@ static void SetupHooks(const FunctionCallbackInfo& args) { Local fn_obj = args[0].As(); -#define SET_HOOK_FN(name) \ +#define SET_HOOK_FN(name) \ Local name##_v = fn_obj->Get( \ env->context(), \ FIXED_ONE_BYTE_STRING(env->isolate(), #name)).ToLocalChecked(); \ - CHECK(name##_v->IsFunction()); \ + CHECK(name##_v->IsFunction()); \ env->set_async_hooks_##name##_function(name##_v.As()); SET_HOOK_FN(init); @@ -336,8 +336,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { // The per-Isolate API provides no way of knowing whether there are multiple // users of the PromiseHook. That hopefully goes away when V8 introduces // a per-context API. - Isolate* isolate = static_cast(data); - isolate->SetPromiseHook(nullptr); + Isolate* isolate = static_cast(data); + isolate->SetPromiseHook(nullptr); }, static_cast(isolate)); } @@ -359,7 +359,7 @@ void AsyncWrap::WeakCallback(const WeakCallbackInfo& info) { Local val; if (!prop_bag->Get(p->env->context(), p->env->destroyed_string()) - .ToLocal(&val)) { + .ToLocal(&val)) { return; } @@ -387,7 +387,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo& args) { void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { AsyncWrap* wrap; - args.GetReturnValue().Set(-1); + args.GetReturnValue().Set(invalid_async_id); ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); args.GetReturnValue().Set(wrap->get_async_id()); } @@ -414,16 +414,21 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { AsyncWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); double execution_async_id = - args[0]->IsNumber() ? args[0].As()->Value() : -1; + args[0]->IsNumber() ? args[0].As()->Value() : invalid_async_id; wrap->AsyncReset(execution_async_id); } +void AsyncWrap::EmitDestroy() { + AsyncWrap::EmitDestroy(env(), async_id_); + // Ensure no double destroy is emitted via AsyncReset(). + async_id_ = invalid_async_id; +} void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { CHECK(args[0]->IsNumber()); AsyncWrap::EmitDestroy( Environment::GetCurrent(args), - args[0].As()->Value()); + args[0].As()->Value()); } Local AsyncWrap::GetConstructorTemplate(Environment* env) { @@ -457,10 +462,10 @@ void AsyncWrap::Initialize(Local target, PropertyAttribute ReadOnlyDontDelete = static_cast(ReadOnly | DontDelete); -#define FORCE_SET_TARGET_FIELD(obj, str, field) \ +#define FORCE_SET_TARGET_FIELD(obj, str, field) \ (obj)->DefineOwnProperty(context, \ - FIXED_ONE_BYTE_STRING(isolate, str), \ - field, \ + FIXED_ONE_BYTE_STRING(isolate, str), \ + field, \ ReadOnlyDontDelete).FromJust() // Attach the uint32_t[] where each slot contains the count of the number of @@ -480,22 +485,22 @@ void AsyncWrap::Initialize(Local target, // kDefaultTriggerAsyncId: Write the id of the resource responsible for a // handle's creation just before calling the new handle's constructor. // After the new handle is constructed kDefaultTriggerAsyncId is set back - // to -1. + // to invalid_async_id. FORCE_SET_TARGET_FIELD(target, "async_id_fields", env->async_hooks()->async_id_fields().GetJSArray()); target->Set(context, - env->async_ids_stack_string(), + env->async_ids_stack_string(), env->async_hooks()->async_ids_stack().GetJSArray()).Check(); target->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), + FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), env->owner_symbol()).Check(); Local constants = Object::New(isolate); -#define SET_HOOKS_CONSTANT(name) \ - FORCE_SET_TARGET_FIELD( \ +#define SET_HOOKS_CONSTANT(name) \ + FORCE_SET_TARGET_FIELD( \ constants, #name, Integer::New(isolate, AsyncHooks::name)); SET_HOOKS_CONSTANT(kInit); @@ -514,9 +519,9 @@ void AsyncWrap::Initialize(Local target, FORCE_SET_TARGET_FIELD(target, "constants", constants); Local async_providers = Object::New(isolate); -#define V(p) \ - FORCE_SET_TARGET_FIELD( \ - async_providers, #p, Integer::New(isolate, AsyncWrap::PROVIDER_ ## p)); +#define V(p) \ + FORCE_SET_TARGET_FIELD( \ + async_providers, #p, Integer::New(isolate, AsyncWrap::PROVIDER_##p)); NODE_ASYNC_PROVIDER_TYPES(V) #undef V FORCE_SET_TARGET_FIELD(target, "Providers", async_providers); @@ -547,13 +552,6 @@ void AsyncWrap::Initialize(Local target, } } - -AsyncWrap::AsyncWrap(Environment* env, - Local object, - ProviderType provider, - double execution_async_id) - : AsyncWrap(env, object, provider, execution_async_id, false) {} - AsyncWrap::AsyncWrap(Environment* env, Local object, ProviderType provider, @@ -568,22 +566,21 @@ AsyncWrap::AsyncWrap(Environment* env, AsyncReset(execution_async_id, silent); } - AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); - EmitDestroy(env(), get_async_id()); + EmitDestroy(); } void AsyncWrap::EmitTraceEventDestroy() { switch (provider_type()) { - #define V(PROVIDER) \ - case PROVIDER_ ## PROVIDER: \ +#define V(PROVIDER) \ + case PROVIDER_##PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_END0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER, static_cast(get_async_id())); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) - #undef V +#undef V default: UNREACHABLE(); } @@ -603,15 +600,17 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { } void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { - AsyncReset(object(), execution_async_id, silent); + AsyncReset(object(), false, 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(Local resource, double execution_async_id, +void AsyncWrap::AsyncReset(Local resource, + bool skip_destroy, + double execution_async_id, bool silent) { - if (async_id_ != -1) { + if (!skip_destroy && (async_id_ != invalid_async_id)) { // 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 // before generating a new async_id. @@ -619,26 +618,26 @@ void AsyncWrap::AsyncReset(Local resource, double execution_async_id, } // Now we can assign a new async_id_ to this instance. - async_id_ = - execution_async_id == -1 ? env()->new_async_id() : execution_async_id; + async_id_ = execution_async_id == invalid_async_id ? env()->new_async_id() + : execution_async_id; trigger_async_id_ = env()->get_default_trigger_async_id(); switch (provider_type()) { -#define V(PROVIDER) \ - case PROVIDER_ ## PROVIDER: \ - if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED( \ - TRACING_CATEGORY_NODE1(async_hooks))) { \ - auto data = tracing::TracedValue::Create(); \ - data->SetInteger("executionAsyncId", \ - static_cast(env()->execution_async_id())); \ - data->SetInteger("triggerAsyncId", \ - static_cast(get_trigger_async_id())); \ +#define V(PROVIDER) \ + case PROVIDER_##PROVIDER: \ + if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED( \ + TRACING_CATEGORY_NODE1(async_hooks))) { \ + auto data = tracing::TracedValue::Create(); \ + data->SetInteger("executionAsyncId", \ + static_cast(env()->execution_async_id())); \ + data->SetInteger("triggerAsyncId", \ + static_cast(get_trigger_async_id())); \ TRACE_EVENT_NESTABLE_ASYNC_BEGIN1( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER, static_cast(get_async_id()), \ "data", std::move(data)); \ - } \ - break; + } \ + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -671,10 +670,10 @@ void AsyncWrap::EmitAsyncInit(Environment* env, Local init_fn = env->async_hooks_init_function(); Local argv[] = { - Number::New(env->isolate(), async_id), - type, - Number::New(env->isolate(), trigger_async_id), - object, + Number::New(env->isolate(), async_id), + type, + Number::New(env->isolate(), trigger_async_id), + object, }; TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); @@ -688,7 +687,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, EmitTraceEventBefore(); ProviderType provider = provider_type(); - async_context context { get_async_id(), get_trigger_async_id() }; + async_context context{get_async_id(), get_trigger_async_id()}; MaybeLocal ret = InternalMakeCallback( env(), object(), cb, argc, argv, context); @@ -705,7 +704,7 @@ std::string AsyncWrap::MemoryInfoName() const { std::string AsyncWrap::diagnostic_name() const { return MemoryInfoName() + " (" + std::to_string(env()->thread_id()) + ":" + - std::to_string(static_cast(async_id_)) + ")"; + std::to_string(static_cast(async_id_)) + ")"; } Local AsyncWrap::GetOwner() { diff --git a/src/async_wrap.h b/src/async_wrap.h index bcd37bb0c0d5b6..9176cf8c35e976 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -31,51 +31,51 @@ namespace node { -#define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ - V(NONE) \ - V(DNSCHANNEL) \ - V(FILEHANDLE) \ - V(FILEHANDLECLOSEREQ) \ - V(FSEVENTWRAP) \ - V(FSREQCALLBACK) \ - V(FSREQPROMISE) \ - V(GETADDRINFOREQWRAP) \ - V(GETNAMEINFOREQWRAP) \ - V(HEAPSNAPSHOT) \ - V(HTTP2SESSION) \ - V(HTTP2STREAM) \ - V(HTTP2PING) \ - V(HTTP2SETTINGS) \ - V(HTTPINCOMINGMESSAGE) \ - V(HTTPCLIENTREQUEST) \ - V(JSSTREAM) \ - V(MESSAGEPORT) \ - V(PIPECONNECTWRAP) \ - V(PIPESERVERWRAP) \ - V(PIPEWRAP) \ - V(PROCESSWRAP) \ - V(PROMISE) \ - V(QUERYWRAP) \ - V(SHUTDOWNWRAP) \ - V(SIGNALWRAP) \ - V(STATWATCHER) \ - V(STREAMPIPE) \ - V(TCPCONNECTWRAP) \ - V(TCPSERVERWRAP) \ - V(TCPWRAP) \ - V(TTYWRAP) \ - V(UDPSENDWRAP) \ - V(UDPWRAP) \ - V(WORKER) \ - V(WRITEWRAP) \ +#define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ + V(NONE) \ + V(DNSCHANNEL) \ + V(FILEHANDLE) \ + V(FILEHANDLECLOSEREQ) \ + V(FSEVENTWRAP) \ + V(FSREQCALLBACK) \ + V(FSREQPROMISE) \ + V(GETADDRINFOREQWRAP) \ + V(GETNAMEINFOREQWRAP) \ + V(HEAPSNAPSHOT) \ + V(HTTP2SESSION) \ + V(HTTP2STREAM) \ + V(HTTP2PING) \ + V(HTTP2SETTINGS) \ + V(HTTPINCOMINGMESSAGE) \ + V(HTTPCLIENTREQUEST) \ + V(JSSTREAM) \ + V(MESSAGEPORT) \ + V(PIPECONNECTWRAP) \ + V(PIPESERVERWRAP) \ + V(PIPEWRAP) \ + V(PROCESSWRAP) \ + V(PROMISE) \ + V(QUERYWRAP) \ + V(SHUTDOWNWRAP) \ + V(SIGNALWRAP) \ + V(STATWATCHER) \ + V(STREAMPIPE) \ + V(TCPCONNECTWRAP) \ + V(TCPSERVERWRAP) \ + V(TCPWRAP) \ + V(TTYWRAP) \ + V(UDPSENDWRAP) \ + V(UDPWRAP) \ + V(WORKER) \ + V(WRITEWRAP) \ V(ZLIB) #if HAVE_OPENSSL -#define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ - V(PBKDF2REQUEST) \ - V(KEYPAIRGENREQUEST) \ - V(RANDOMBYTESREQUEST) \ - V(SCRYPTREQUEST) \ +#define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ + V(PBKDF2REQUEST) \ + V(KEYPAIRGENREQUEST) \ + V(RANDOMBYTESREQUEST) \ + V(SCRYPTREQUEST) \ V(TLSWRAP) #else #define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) @@ -88,9 +88,9 @@ namespace node { #define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V) #endif // HAVE_INSPECTOR -#define NODE_ASYNC_PROVIDER_TYPES(V) \ - NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ - NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ +#define NODE_ASYNC_PROVIDER_TYPES(V) \ + NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ + NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V) class Environment; @@ -103,18 +103,21 @@ class AsyncWrap : public BaseObject { PROVIDER_ ## PROVIDER, NODE_ASYNC_PROVIDER_TYPES(V) #undef V - PROVIDERS_LENGTH, + PROVIDERS_LENGTH, }; AsyncWrap(Environment* env, v8::Local object, ProviderType provider, - double execution_async_id = -1); + double execution_async_id = invalid_async_id, + bool silent = false); ~AsyncWrap() override; AsyncWrap() = delete; + static constexpr double invalid_async_id = -1; + static v8::Local GetConstructorTemplate( Environment* env); @@ -128,7 +131,7 @@ class AsyncWrap : public BaseObject { static void PopAsyncIds(const v8::FunctionCallbackInfo& args); static void AsyncReset(const v8::FunctionCallbackInfo& args); static void QueueDestroyAsyncId( - const v8::FunctionCallbackInfo& args); + const v8::FunctionCallbackInfo& args); static void EmitAsyncInit(Environment* env, v8::Local object, @@ -141,6 +144,8 @@ class AsyncWrap : public BaseObject { static void EmitAfter(Environment* env, double async_id); static void EmitPromiseResolve(Environment* env, double async_id); + void EmitDestroy(); + void EmitTraceEventBefore(); static void EmitTraceEventAfter(ProviderType type, double async_id); void EmitTraceEventDestroy(); @@ -155,10 +160,12 @@ class AsyncWrap : public BaseObject { inline double get_trigger_async_id() const; void AsyncReset(v8::Local resource, - double execution_async_id = -1, + bool skip_destroy, + double execution_async_id = invalid_async_id, bool silent = false); - void AsyncReset(double execution_async_id = -1, bool silent = false); + void AsyncReset(double execution_async_id = invalid_async_id, + bool silent = false); // Only call these within a valid HandleScope. v8::MaybeLocal MakeCallback(const v8::Local cb, @@ -180,7 +187,7 @@ class AsyncWrap : public BaseObject { virtual std::string diagnostic_name() const; std::string MemoryInfoName() const override; - static void WeakCallback(const v8::WeakCallbackInfo &info); + static void WeakCallback(const v8::WeakCallbackInfo& info); // Returns the object that 'owns' an async wrap. For example, for a // TCP connection handle, this is the corresponding net.Socket. @@ -201,16 +208,9 @@ class AsyncWrap : public BaseObject { }; private: - friend class PromiseWrap; - - AsyncWrap(Environment* env, - v8::Local promise, - ProviderType provider, - double execution_async_id, - bool silent); ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. - double async_id_ = -1; + double async_id_ = invalid_async_id; double trigger_async_id_; }; diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index a8a8db554783fb..6125f916f0aebf 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -158,7 +158,8 @@ class Parser : public AsyncWrap, public StreamListener { : AsyncWrap(env, wrap, type == HTTP_REQUEST ? AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE : - AsyncWrap::PROVIDER_HTTPCLIENTREQUEST), + AsyncWrap::PROVIDER_HTTPCLIENTREQUEST, + AsyncWrap::invalid_async_id, true), current_buffer_len_(0), current_buffer_data_(nullptr) { Init(type); @@ -443,14 +444,13 @@ class Parser : public AsyncWrap, public StreamListener { static void Free(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); Parser* parser; ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder()); // Since the Parser destructor isn't going to run the destroy() callbacks // it needs to be triggered manually. parser->EmitTraceEventDestroy(); - parser->EmitDestroy(env, parser->get_async_id()); + parser->EmitDestroy(); } @@ -526,6 +526,7 @@ class Parser : public AsyncWrap, public StreamListener { : AsyncWrap::PROVIDER_HTTPCLIENTREQUEST); parser->set_provider_type(provider); + parser->AsyncReset(args[1].As(), true); parser->Init(type); } diff --git a/test/async-hooks/coverage.md b/test/async-hooks/coverage.md index 0a2af0d06bfd62..1ba18a938330a9 100644 --- a/test/async-hooks/coverage.md +++ b/test/async-hooks/coverage.md @@ -9,7 +9,8 @@ Showing which kind of async resource is covered by which test: | FSREQCALLBACK | test-fsreqcallback-{access,readFile}.js | | GETADDRINFOREQWRAP | test-getaddrinforeqwrap.js | | GETNAMEINFOREQWRAP | test-getnameinforeqwrap.js | -| HTTPPARSER | test-httpparser.{request,response}.js | +| HTTPINCOMINGMESSAGE | test-httpparser.request.js | +| HTTPCLIENTREQUEST | test-httpparser.response.js | | Immediate | test-immediate.js | | JSSTREAM | TODO (crashes when accessing directly) | | PBKDF2REQUEST | test-crypto-pbkdf2.js | diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js index 3461974ef9c477..55b9b055a0f050 100644 --- a/test/async-hooks/test-graph.http.js +++ b/test/async-hooks/test-graph.http.js @@ -35,13 +35,13 @@ process.on('exit', function() { { type: 'TCPCONNECTWRAP', id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' }, - { type: 'HTTPPARSER', - id: 'httpparser:1', + { type: 'HTTPCLIENTREQUEST', + id: 'httpclientrequest:1', triggerAsyncId: 'tcpserver:1' }, { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' }, { type: 'Timeout', id: 'timeout:1', triggerAsyncId: 'tcp:2' }, - { type: 'HTTPPARSER', - id: 'httpparser:2', + { type: 'HTTPINCOMINGMESSAGE', + id: 'httpincomingmessage:1', triggerAsyncId: 'tcp:2' }, { type: 'Timeout', id: 'timeout:2', diff --git a/test/async-hooks/test-graph.tls-write.js b/test/async-hooks/test-graph.tls-write.js index 580264316dea90..5aee38e6b6841a 100644 --- a/test/async-hooks/test-graph.tls-write.js +++ b/test/async-hooks/test-graph.tls-write.js @@ -64,12 +64,8 @@ function onexit() { id: 'getaddrinforeq:1', triggerAsyncId: 'tls:1' }, { type: 'TCPCONNECTWRAP', id: 'tcpconnect:1', triggerAsyncId: 'tcp:1' }, - { type: 'WRITEWRAP', id: 'write:1', triggerAsyncId: 'tcpconnect:1' }, { type: 'TCPWRAP', id: 'tcp:2', triggerAsyncId: 'tcpserver:1' }, { type: 'TLSWRAP', id: 'tls:2', triggerAsyncId: 'tcpserver:1' }, - { type: 'WRITEWRAP', id: 'write:2', triggerAsyncId: null }, - { type: 'WRITEWRAP', id: 'write:3', triggerAsyncId: null }, - { type: 'WRITEWRAP', id: 'write:4', triggerAsyncId: null }, { type: 'Immediate', id: 'immediate:1', triggerAsyncId: 'tcp:2' }, { type: 'Immediate', id: 'immediate:2', triggerAsyncId: 'tcp:1' }, ] diff --git a/test/async-hooks/test-httparser-reuse.js b/test/async-hooks/test-httparser-reuse.js index b6d82d7d5e9087..06441562e05aa8 100644 --- a/test/async-hooks/test-httparser-reuse.js +++ b/test/async-hooks/test-httparser-reuse.js @@ -1,28 +1,54 @@ 'use strict'; const common = require('../common'); -const http = require('http'); const assert = require('assert'); const { createHook } = require('async_hooks'); +const http = require('http'); + +// Verify that resource emitted for an HTTPParser is not reused. +// Verify that correct create/destroy events are emitted. + const reused = Symbol('reused'); -let reusedHTTPParser = false; -const asyncHook = createHook({ +const reusedParser = []; +const incomingMessageParser = []; +const clientRequestParser = []; +const dupDestroys = []; +const destroyed = []; + +createHook({ init(asyncId, type, triggerAsyncId, resource) { + switch (type) { + case 'HTTPINCOMINGMESSAGE': + incomingMessageParser.push(asyncId); + break; + case 'HTTPCLIENTREQUEST': + clientRequestParser.push(asyncId); + break; + } + if (resource[reused]) { - reusedHTTPParser = true; + reusedParser.push( + `resource reused: ${asyncId}, ${triggerAsyncId}, ${type}` + ); } resource[reused] = true; + }, + destroy(asyncId) { + if (destroyed.includes(asyncId)) { + dupDestroys.push(asyncId); + } else { + destroyed.push(asyncId); + } } -}); -asyncHook.enable(); +}).enable(); -const server = http.createServer(function(req, res) { +const server = http.createServer((req, res) => { res.end(); }); const PORT = 3000; -const url = 'http://127.0.0.1:' + PORT; +const url = `http://127.0.0.1:${PORT}`; server.listen(PORT, common.mustCall(() => { http.get(url, common.mustCall(() => { @@ -30,10 +56,21 @@ server.listen(PORT, common.mustCall(() => { server.listen(PORT, common.mustCall(() => { http.get(url, common.mustCall(() => { server.close(common.mustCall(() => { - assert.strictEqual(reusedHTTPParser, false); + setTimeout(common.mustCall(verify), 200); })); })); })); })); })); })); + +function verify() { + assert.strictEqual(reusedParser.length, 0); + + assert.strictEqual(incomingMessageParser.length, 2); + assert.strictEqual(clientRequestParser.length, 2); + + assert.strictEqual(dupDestroys.length, 0); + incomingMessageParser.forEach((id) => assert.ok(destroyed.includes(id))); + clientRequestParser.forEach((id) => assert.ok(destroyed.includes(id))); +} diff --git a/test/async-hooks/test-httpparser.request.js b/test/async-hooks/test-httpparser.request.js index 7ba8feefe34d5d..810e9d21b363cc 100644 --- a/test/async-hooks/test-httpparser.request.js +++ b/test/async-hooks/test-httpparser.request.js @@ -21,6 +21,7 @@ const request = Buffer.from( ); const parser = new HTTPParser(REQUEST); +parser.initialize(REQUEST, {}); const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE'); const httpparser = as[0]; diff --git a/test/async-hooks/test-httpparser.response.js b/test/async-hooks/test-httpparser.response.js index 85d6f76525d49b..b3be63562b5122 100644 --- a/test/async-hooks/test-httpparser.response.js +++ b/test/async-hooks/test-httpparser.response.js @@ -26,6 +26,7 @@ const request = Buffer.from( ); const parser = new HTTPParser(RESPONSE); +parser.initialize(RESPONSE, {}); const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST'); const httpparser = as[0]; diff --git a/test/async-hooks/verify-graph.js b/test/async-hooks/verify-graph.js index d1e09f92bc4adf..1b188faa149b21 100644 --- a/test/async-hooks/verify-graph.js +++ b/test/async-hooks/verify-graph.js @@ -98,6 +98,18 @@ module.exports = function verifyGraph(hooks, graph) { ); } assert.strictEqual(errors.length, 0); + + // Verify that all expected types are present + const expTypes = Object.create(null); + for (let i = 0; i < graph.length; i++) { + if (expTypes[graph[i].type] == null) expTypes[graph[i].type] = 0; + expTypes[graph[i].type]++; + } + + for (const type in expTypes) { + assert.strictEqual(typeSeen[type], expTypes[type], + `Expecting type '${type}' in graph`); + } }; // diff --git a/test/parallel/test-async-hooks-http-parser-destroy.js b/test/parallel/test-async-hooks-http-parser-destroy.js index d2e1071c280d66..d69c474c1d371e 100644 --- a/test/parallel/test-async-hooks-http-parser-destroy.js +++ b/test/parallel/test-async-hooks-http-parser-destroy.js @@ -16,7 +16,7 @@ const createdIds = []; const destroyedIds = []; async_hooks.createHook({ init: common.mustCallAtLeast((asyncId, type) => { - if (type === 'HTTPPARSER') { + if (type === 'HTTPINCOMINGMESSAGE' || type === 'HTTPCLIENTREQUEST') { createdIds.push(asyncId); } }, N), @@ -25,7 +25,7 @@ async_hooks.createHook({ } }).enable(); -const server = http.createServer(function(req, res) { +const server = http.createServer((req, res) => { res.end('Hello'); }); @@ -39,6 +39,7 @@ const countdown = new Countdown(N, () => { // Give the server sockets time to close (which will also free their // associated parser objects) after the server has been closed. setTimeout(() => { + assert.strictEqual(createdIds.length, 2 * N); createdIds.forEach((createdAsyncId) => { assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0); }); From 40842c680440c36e4071706d3be4bdebb76cbd0a Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Mon, 29 Apr 2019 14:34:38 +0200 Subject: [PATCH 2/7] make initialize() mandatory --- lib/_http_common.js | 2 +- src/async_wrap.cc | 2 ++ src/node_http_parser_impl.h | 19 +++++++------------ test/async-hooks/test-httpparser.request.js | 2 +- test/async-hooks/test-httpparser.response.js | 2 +- test/parallel/test-http-parser-bad-ref.js | 3 ++- test/parallel/test-http-parser-lazy-loaded.js | 4 ++-- test/parallel/test-http-parser.js | 5 +++-- test/sequential/test-async-wrap-getasyncid.js | 2 +- test/sequential/test-http-regr-gh-2928.js | 3 +-- 10 files changed, 21 insertions(+), 23 deletions(-) diff --git a/lib/_http_common.js b/lib/_http_common.js index 7772ec69314ada..75995260128862 100644 --- a/lib/_http_common.js +++ b/lib/_http_common.js @@ -156,7 +156,7 @@ function parserOnMessageComplete() { const parsers = new FreeList('parsers', 1000, function parsersCb() { - const parser = new HTTPParser(HTTPParser.REQUEST); + const parser = new HTTPParser(); cleanParser(parser); diff --git a/src/async_wrap.cc b/src/async_wrap.cc index f7fa7a5c61b2e7..95e0c5956844d9 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -610,6 +610,8 @@ void AsyncWrap::AsyncReset(Local resource, bool skip_destroy, double execution_async_id, bool silent) { + CHECK_NE(provider_type(), PROVIDER_NONE); + if (!skip_destroy && (async_id_ != invalid_async_id)) { // 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 diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 6125f916f0aebf..7f3dfc9f3ca0c0 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -154,18 +154,17 @@ struct StringPtr { class Parser : public AsyncWrap, public StreamListener { public: - Parser(Environment* env, Local wrap, parser_type_t type) - : AsyncWrap(env, wrap, - type == HTTP_REQUEST ? - AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE : - AsyncWrap::PROVIDER_HTTPCLIENTREQUEST, + Parser(Environment* env, Local wrap) + : AsyncWrap(env, + wrap, + // AsyncWrap::PROVIDER_NONE would match better here but + // there is an assert in AsyncWrap() which avoid this. + AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE, AsyncWrap::invalid_async_id, true), current_buffer_len_(0), current_buffer_data_(nullptr) { - Init(type); } - void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("current_buffer", current_buffer_); } @@ -427,11 +426,7 @@ class Parser : public AsyncWrap, public StreamListener { static void New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - CHECK(args[0]->IsInt32()); - parser_type_t type = - static_cast(args[0].As()->Value()); - CHECK(type == HTTP_REQUEST || type == HTTP_RESPONSE); - new Parser(env, args.This(), type); + new Parser(env, args.This()); } diff --git a/test/async-hooks/test-httpparser.request.js b/test/async-hooks/test-httpparser.request.js index 810e9d21b363cc..f4552398d38e8f 100644 --- a/test/async-hooks/test-httpparser.request.js +++ b/test/async-hooks/test-httpparser.request.js @@ -20,7 +20,7 @@ const request = Buffer.from( 'GET /hello HTTP/1.1\r\n\r\n' ); -const parser = new HTTPParser(REQUEST); +const parser = new HTTPParser(); parser.initialize(REQUEST, {}); const as = hooks.activitiesOfTypes('HTTPINCOMINGMESSAGE'); const httpparser = as[0]; diff --git a/test/async-hooks/test-httpparser.response.js b/test/async-hooks/test-httpparser.response.js index b3be63562b5122..a207a62636f291 100644 --- a/test/async-hooks/test-httpparser.response.js +++ b/test/async-hooks/test-httpparser.response.js @@ -25,7 +25,7 @@ const request = Buffer.from( 'pong' ); -const parser = new HTTPParser(RESPONSE); +const parser = new HTTPParser(); parser.initialize(RESPONSE, {}); const as = hooks.activitiesOfTypes('HTTPCLIENTREQUEST'); const httpparser = as[0]; diff --git a/test/parallel/test-http-parser-bad-ref.js b/test/parallel/test-http-parser-bad-ref.js index 0b132d69a2dc96..2c1bfe67485db7 100644 --- a/test/parallel/test-http-parser-bad-ref.js +++ b/test/parallel/test-http-parser-bad-ref.js @@ -24,7 +24,8 @@ function flushPool() { function demoBug(part1, part2) { flushPool(); - const parser = new HTTPParser(HTTPParser.REQUEST); + const parser = new HTTPParser(); + parser.initialize(HTTPParser.REQUEST, {}); parser.headers = []; parser.url = ''; diff --git a/test/parallel/test-http-parser-lazy-loaded.js b/test/parallel/test-http-parser-lazy-loaded.js index 6d6b2ddd256bd4..34e2fdbf559ab4 100644 --- a/test/parallel/test-http-parser-lazy-loaded.js +++ b/test/parallel/test-http-parser-lazy-loaded.js @@ -7,8 +7,8 @@ const { getOptionValue } = require('internal/options'); // Monkey patch before requiring anything class DummyParser { - constructor(type) { - this.test_type = type; + constructor() { + this.test_type = DummyParser.REQUEST; } } DummyParser.REQUEST = Symbol(); diff --git a/test/parallel/test-http-parser.js b/test/parallel/test-http-parser.js index 078895d49b13aa..97dc57f755ad88 100644 --- a/test/parallel/test-http-parser.js +++ b/test/parallel/test-http-parser.js @@ -38,7 +38,8 @@ const kOnMessageComplete = HTTPParser.kOnMessageComplete | 0; function newParser(type) { - const parser = new HTTPParser(type); + const parser = new HTTPParser(); + parser.initialize(type, {}); parser.headers = []; parser.url = ''; @@ -95,7 +96,7 @@ function expectBody(expected) { throw new Error('hello world'); }; - parser.initialize(HTTPParser.REQUEST, request); + parser.initialize(REQUEST, {}); assert.throws( () => { parser.execute(request, 0, request.length); }, diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index e3cfd6bef01bc9..fc9dce6d4b2f99 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -152,7 +152,7 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check { const { HTTPParser } = require('_http_common'); - testInitialized(new HTTPParser(HTTPParser.REQUEST), 'HTTPParser'); + 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 400dc02013325d..5111e234d168f5 100644 --- a/test/sequential/test-http-regr-gh-2928.js +++ b/test/sequential/test-http-regr-gh-2928.js @@ -7,7 +7,6 @@ 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; @@ -25,7 +24,7 @@ function execAndClose() { process.stdout.write('.'); const parser = parsers.pop(); - parser.initialize(HTTPParser.RESPONSE, new AsyncResource('ClientRequest')); + parser.initialize(HTTPParser.RESPONSE, {}); const socket = net.connect(common.PORT); socket.on('error', (e) => { From b4c189d30917ebe5445bb08cd37765dc729d1836 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Mon, 29 Apr 2019 16:04:34 +0200 Subject: [PATCH 3/7] fixup: revert whitespace changes --- src/async_wrap.cc | 91 ++++++++++++++++++----------------- src/async_wrap.h | 96 ++++++++++++++++++------------------- src/node_http_parser_impl.h | 1 + 3 files changed, 95 insertions(+), 93 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 95e0c5956844d9..2ab397b36b2d7a 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -63,7 +63,7 @@ namespace node { static const char* const provider_names[] = { #define V(PROVIDER) \ #PROVIDER, - NODE_ASYNC_PROVIDER_TYPES(V) + NODE_ASYNC_PROVIDER_TYPES(V) #undef V }; @@ -131,12 +131,12 @@ void AsyncWrap::EmitPromiseResolve(Environment* env, double async_id) { void AsyncWrap::EmitTraceEventBefore() { switch (provider_type()) { -#define V(PROVIDER) \ - case PROVIDER_##PROVIDER: \ +#define V(PROVIDER) \ + case PROVIDER_ ## PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_BEGIN0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER "_CALLBACK", static_cast(get_async_id())); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -153,12 +153,12 @@ void AsyncWrap::EmitBefore(Environment* env, double async_id) { void AsyncWrap::EmitTraceEventAfter(ProviderType type, double async_id) { switch (type) { -#define V(PROVIDER) \ - case PROVIDER_##PROVIDER: \ +#define V(PROVIDER) \ + case PROVIDER_ ## PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_END0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER "_CALLBACK", static_cast(async_id)); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -214,7 +214,7 @@ PromiseWrap* PromiseWrap::New(Environment* env, void PromiseWrap::getIsChainedPromise(Local property, const PropertyCallbackInfo& info) { info.GetReturnValue().Set( - info.Holder()->GetInternalField(kIsChainedPromiseField)); + info.Holder()->GetInternalField(kIsChainedPromiseField)); } static PromiseWrap* extractPromiseWrap(Local promise) { @@ -293,11 +293,11 @@ static void SetupHooks(const FunctionCallbackInfo& args) { Local fn_obj = args[0].As(); -#define SET_HOOK_FN(name) \ +#define SET_HOOK_FN(name) \ Local name##_v = fn_obj->Get( \ env->context(), \ FIXED_ONE_BYTE_STRING(env->isolate(), #name)).ToLocalChecked(); \ - CHECK(name##_v->IsFunction()); \ + CHECK(name##_v->IsFunction()); \ env->set_async_hooks_##name##_function(name##_v.As()); SET_HOOK_FN(init); @@ -336,8 +336,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo& args) { // The per-Isolate API provides no way of knowing whether there are multiple // users of the PromiseHook. That hopefully goes away when V8 introduces // a per-context API. - Isolate* isolate = static_cast(data); - isolate->SetPromiseHook(nullptr); + Isolate* isolate = static_cast(data); + isolate->SetPromiseHook(nullptr); }, static_cast(isolate)); } @@ -359,7 +359,7 @@ void AsyncWrap::WeakCallback(const WeakCallbackInfo& info) { Local val; if (!prop_bag->Get(p->env->context(), p->env->destroyed_string()) - .ToLocal(&val)) { + .ToLocal(&val)) { return; } @@ -428,7 +428,7 @@ void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { CHECK(args[0]->IsNumber()); AsyncWrap::EmitDestroy( Environment::GetCurrent(args), - args[0].As()->Value()); + args[0].As()->Value()); } Local AsyncWrap::GetConstructorTemplate(Environment* env) { @@ -462,10 +462,10 @@ void AsyncWrap::Initialize(Local target, PropertyAttribute ReadOnlyDontDelete = static_cast(ReadOnly | DontDelete); -#define FORCE_SET_TARGET_FIELD(obj, str, field) \ +#define FORCE_SET_TARGET_FIELD(obj, str, field) \ (obj)->DefineOwnProperty(context, \ - FIXED_ONE_BYTE_STRING(isolate, str), \ - field, \ + FIXED_ONE_BYTE_STRING(isolate, str), \ + field, \ ReadOnlyDontDelete).FromJust() // Attach the uint32_t[] where each slot contains the count of the number of @@ -491,16 +491,16 @@ void AsyncWrap::Initialize(Local target, env->async_hooks()->async_id_fields().GetJSArray()); target->Set(context, - env->async_ids_stack_string(), + env->async_ids_stack_string(), env->async_hooks()->async_ids_stack().GetJSArray()).Check(); target->Set(context, - FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), + FIXED_ONE_BYTE_STRING(env->isolate(), "owner_symbol"), env->owner_symbol()).Check(); Local constants = Object::New(isolate); -#define SET_HOOKS_CONSTANT(name) \ - FORCE_SET_TARGET_FIELD( \ +#define SET_HOOKS_CONSTANT(name) \ + FORCE_SET_TARGET_FIELD( \ constants, #name, Integer::New(isolate, AsyncHooks::name)); SET_HOOKS_CONSTANT(kInit); @@ -519,9 +519,9 @@ void AsyncWrap::Initialize(Local target, FORCE_SET_TARGET_FIELD(target, "constants", constants); Local async_providers = Object::New(isolate); -#define V(p) \ - FORCE_SET_TARGET_FIELD( \ - async_providers, #p, Integer::New(isolate, AsyncWrap::PROVIDER_##p)); +#define V(p) \ + FORCE_SET_TARGET_FIELD( \ + async_providers, #p, Integer::New(isolate, AsyncWrap::PROVIDER_ ## p)); NODE_ASYNC_PROVIDER_TYPES(V) #undef V FORCE_SET_TARGET_FIELD(target, "Providers", async_providers); @@ -566,6 +566,7 @@ AsyncWrap::AsyncWrap(Environment* env, AsyncReset(execution_async_id, silent); } + AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); EmitDestroy(); @@ -573,14 +574,14 @@ AsyncWrap::~AsyncWrap() { void AsyncWrap::EmitTraceEventDestroy() { switch (provider_type()) { -#define V(PROVIDER) \ - case PROVIDER_##PROVIDER: \ + #define V(PROVIDER) \ + case PROVIDER_ ## PROVIDER: \ TRACE_EVENT_NESTABLE_ASYNC_END0( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER, static_cast(get_async_id())); \ - break; + break; NODE_ASYNC_PROVIDER_TYPES(V) -#undef V + #undef V default: UNREACHABLE(); } @@ -625,21 +626,21 @@ void AsyncWrap::AsyncReset(Local resource, trigger_async_id_ = env()->get_default_trigger_async_id(); switch (provider_type()) { -#define V(PROVIDER) \ - case PROVIDER_##PROVIDER: \ - if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED( \ - TRACING_CATEGORY_NODE1(async_hooks))) { \ - auto data = tracing::TracedValue::Create(); \ - data->SetInteger("executionAsyncId", \ - static_cast(env()->execution_async_id())); \ - data->SetInteger("triggerAsyncId", \ - static_cast(get_trigger_async_id())); \ +#define V(PROVIDER) \ + case PROVIDER_ ## PROVIDER: \ + if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED( \ + TRACING_CATEGORY_NODE1(async_hooks))) { \ + auto data = tracing::TracedValue::Create(); \ + data->SetInteger("executionAsyncId", \ + static_cast(env()->execution_async_id())); \ + data->SetInteger("triggerAsyncId", \ + static_cast(get_trigger_async_id())); \ TRACE_EVENT_NESTABLE_ASYNC_BEGIN1( \ TRACING_CATEGORY_NODE1(async_hooks), \ #PROVIDER, static_cast(get_async_id()), \ "data", std::move(data)); \ - } \ - break; + } \ + break; NODE_ASYNC_PROVIDER_TYPES(V) #undef V default: @@ -672,10 +673,10 @@ void AsyncWrap::EmitAsyncInit(Environment* env, Local init_fn = env->async_hooks_init_function(); Local argv[] = { - Number::New(env->isolate(), async_id), - type, - Number::New(env->isolate(), trigger_async_id), - object, + Number::New(env->isolate(), async_id), + type, + Number::New(env->isolate(), trigger_async_id), + object, }; TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); @@ -689,7 +690,7 @@ MaybeLocal AsyncWrap::MakeCallback(const Local cb, EmitTraceEventBefore(); ProviderType provider = provider_type(); - async_context context{get_async_id(), get_trigger_async_id()}; + async_context context { get_async_id(), get_trigger_async_id() }; MaybeLocal ret = InternalMakeCallback( env(), object(), cb, argc, argv, context); @@ -706,7 +707,7 @@ std::string AsyncWrap::MemoryInfoName() const { std::string AsyncWrap::diagnostic_name() const { return MemoryInfoName() + " (" + std::to_string(env()->thread_id()) + ":" + - std::to_string(static_cast(async_id_)) + ")"; + std::to_string(static_cast(async_id_)) + ")"; } Local AsyncWrap::GetOwner() { diff --git a/src/async_wrap.h b/src/async_wrap.h index 9176cf8c35e976..cbb8d89b5917c7 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -31,51 +31,51 @@ namespace node { -#define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ - V(NONE) \ - V(DNSCHANNEL) \ - V(FILEHANDLE) \ - V(FILEHANDLECLOSEREQ) \ - V(FSEVENTWRAP) \ - V(FSREQCALLBACK) \ - V(FSREQPROMISE) \ - V(GETADDRINFOREQWRAP) \ - V(GETNAMEINFOREQWRAP) \ - V(HEAPSNAPSHOT) \ - V(HTTP2SESSION) \ - V(HTTP2STREAM) \ - V(HTTP2PING) \ - V(HTTP2SETTINGS) \ - V(HTTPINCOMINGMESSAGE) \ - V(HTTPCLIENTREQUEST) \ - V(JSSTREAM) \ - V(MESSAGEPORT) \ - V(PIPECONNECTWRAP) \ - V(PIPESERVERWRAP) \ - V(PIPEWRAP) \ - V(PROCESSWRAP) \ - V(PROMISE) \ - V(QUERYWRAP) \ - V(SHUTDOWNWRAP) \ - V(SIGNALWRAP) \ - V(STATWATCHER) \ - V(STREAMPIPE) \ - V(TCPCONNECTWRAP) \ - V(TCPSERVERWRAP) \ - V(TCPWRAP) \ - V(TTYWRAP) \ - V(UDPSENDWRAP) \ - V(UDPWRAP) \ - V(WORKER) \ - V(WRITEWRAP) \ +#define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ + V(NONE) \ + V(DNSCHANNEL) \ + V(FILEHANDLE) \ + V(FILEHANDLECLOSEREQ) \ + V(FSEVENTWRAP) \ + V(FSREQCALLBACK) \ + V(FSREQPROMISE) \ + V(GETADDRINFOREQWRAP) \ + V(GETNAMEINFOREQWRAP) \ + V(HEAPSNAPSHOT) \ + V(HTTP2SESSION) \ + V(HTTP2STREAM) \ + V(HTTP2PING) \ + V(HTTP2SETTINGS) \ + V(HTTPINCOMINGMESSAGE) \ + V(HTTPCLIENTREQUEST) \ + V(JSSTREAM) \ + V(MESSAGEPORT) \ + V(PIPECONNECTWRAP) \ + V(PIPESERVERWRAP) \ + V(PIPEWRAP) \ + V(PROCESSWRAP) \ + V(PROMISE) \ + V(QUERYWRAP) \ + V(SHUTDOWNWRAP) \ + V(SIGNALWRAP) \ + V(STATWATCHER) \ + V(STREAMPIPE) \ + V(TCPCONNECTWRAP) \ + V(TCPSERVERWRAP) \ + V(TCPWRAP) \ + V(TTYWRAP) \ + V(UDPSENDWRAP) \ + V(UDPWRAP) \ + V(WORKER) \ + V(WRITEWRAP) \ V(ZLIB) #if HAVE_OPENSSL -#define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ - V(PBKDF2REQUEST) \ - V(KEYPAIRGENREQUEST) \ - V(RANDOMBYTESREQUEST) \ - V(SCRYPTREQUEST) \ +#define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ + V(PBKDF2REQUEST) \ + V(KEYPAIRGENREQUEST) \ + V(RANDOMBYTESREQUEST) \ + V(SCRYPTREQUEST) \ V(TLSWRAP) #else #define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) @@ -88,9 +88,9 @@ namespace node { #define NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V) #endif // HAVE_INSPECTOR -#define NODE_ASYNC_PROVIDER_TYPES(V) \ - NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ - NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ +#define NODE_ASYNC_PROVIDER_TYPES(V) \ + NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \ + NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V) \ NODE_ASYNC_INSPECTOR_PROVIDER_TYPES(V) class Environment; @@ -103,7 +103,7 @@ class AsyncWrap : public BaseObject { PROVIDER_ ## PROVIDER, NODE_ASYNC_PROVIDER_TYPES(V) #undef V - PROVIDERS_LENGTH, + PROVIDERS_LENGTH, }; AsyncWrap(Environment* env, @@ -131,7 +131,7 @@ class AsyncWrap : public BaseObject { static void PopAsyncIds(const v8::FunctionCallbackInfo& args); static void AsyncReset(const v8::FunctionCallbackInfo& args); static void QueueDestroyAsyncId( - const v8::FunctionCallbackInfo& args); + const v8::FunctionCallbackInfo& args); static void EmitAsyncInit(Environment* env, v8::Local object, @@ -187,7 +187,7 @@ class AsyncWrap : public BaseObject { virtual std::string diagnostic_name() const; std::string MemoryInfoName() const override; - static void WeakCallback(const v8::WeakCallbackInfo& info); + static void WeakCallback(const v8::WeakCallbackInfo &info); // Returns the object that 'owns' an async wrap. For example, for a // TCP connection handle, this is the corresponding net.Socket. diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 7f3dfc9f3ca0c0..2659bbd6ec684a 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -165,6 +165,7 @@ class Parser : public AsyncWrap, public StreamListener { current_buffer_data_(nullptr) { } + void MemoryInfo(MemoryTracker* tracker) const override { tracker->TrackField("current_buffer", current_buffer_); } From a8bc5f1a883a5ec818d3073083625b9fb9e1f5c0 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Mon, 29 Apr 2019 16:10:22 +0200 Subject: [PATCH 4/7] fixup: rename invalid_async_id to kInvalidAsyncId --- src/async_wrap.cc | 14 +++++++------- src/async_wrap.h | 10 +++++----- src/node_http_parser_impl.h | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 2ab397b36b2d7a..2dd7d808781f95 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -177,7 +177,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { class PromiseWrap : public AsyncWrap { public: PromiseWrap(Environment* env, Local object, bool silent) - : AsyncWrap(env, object, PROVIDER_PROMISE, invalid_async_id, silent) { + : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) { MakeWeak(); } @@ -387,7 +387,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo& args) { void AsyncWrap::GetAsyncId(const FunctionCallbackInfo& args) { AsyncWrap* wrap; - args.GetReturnValue().Set(invalid_async_id); + args.GetReturnValue().Set(kInvalidAsyncId); ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); args.GetReturnValue().Set(wrap->get_async_id()); } @@ -414,14 +414,14 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo& args) { AsyncWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); double execution_async_id = - args[0]->IsNumber() ? args[0].As()->Value() : invalid_async_id; + args[0]->IsNumber() ? args[0].As()->Value() : kInvalidAsyncId; wrap->AsyncReset(execution_async_id); } void AsyncWrap::EmitDestroy() { AsyncWrap::EmitDestroy(env(), async_id_); // Ensure no double destroy is emitted via AsyncReset(). - async_id_ = invalid_async_id; + async_id_ = kInvalidAsyncId; } void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo& args) { @@ -485,7 +485,7 @@ void AsyncWrap::Initialize(Local target, // kDefaultTriggerAsyncId: Write the id of the resource responsible for a // handle's creation just before calling the new handle's constructor. // After the new handle is constructed kDefaultTriggerAsyncId is set back - // to invalid_async_id. + // to kInvalidAsyncId. FORCE_SET_TARGET_FIELD(target, "async_id_fields", env->async_hooks()->async_id_fields().GetJSArray()); @@ -613,7 +613,7 @@ void AsyncWrap::AsyncReset(Local resource, bool silent) { CHECK_NE(provider_type(), PROVIDER_NONE); - if (!skip_destroy && (async_id_ != invalid_async_id)) { + if (!skip_destroy && (async_id_ != kInvalidAsyncId)) { // 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 // before generating a new async_id. @@ -621,7 +621,7 @@ void AsyncWrap::AsyncReset(Local resource, } // Now we can assign a new async_id_ to this instance. - async_id_ = execution_async_id == invalid_async_id ? env()->new_async_id() + async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id() : execution_async_id; trigger_async_id_ = env()->get_default_trigger_async_id(); diff --git a/src/async_wrap.h b/src/async_wrap.h index cbb8d89b5917c7..f709d767ea8718 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -109,14 +109,14 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, ProviderType provider, - double execution_async_id = invalid_async_id, + double execution_async_id = kInvalidAsyncId, bool silent = false); ~AsyncWrap() override; AsyncWrap() = delete; - static constexpr double invalid_async_id = -1; + static constexpr double kInvalidAsyncId = -1; static v8::Local GetConstructorTemplate( Environment* env); @@ -161,10 +161,10 @@ class AsyncWrap : public BaseObject { void AsyncReset(v8::Local resource, bool skip_destroy, - double execution_async_id = invalid_async_id, + double execution_async_id = kInvalidAsyncId, bool silent = false); - void AsyncReset(double execution_async_id = invalid_async_id, + void AsyncReset(double execution_async_id = kInvalidAsyncId, bool silent = false); // Only call these within a valid HandleScope. @@ -210,7 +210,7 @@ class AsyncWrap : public BaseObject { private: ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. - double async_id_ = invalid_async_id; + double async_id_ = kInvalidAsyncId; double trigger_async_id_; }; diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 2659bbd6ec684a..96139764bd7602 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -160,7 +160,7 @@ class Parser : public AsyncWrap, public StreamListener { // AsyncWrap::PROVIDER_NONE would match better here but // there is an assert in AsyncWrap() which avoid this. AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE, - AsyncWrap::invalid_async_id, true), + AsyncWrap::kInvalidAsyncId, true), current_buffer_len_(0), current_buffer_data_(nullptr) { } From 0d790d2269256da73277ec86245f116f5b18a964 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Mon, 29 Apr 2019 20:35:36 +0200 Subject: [PATCH 5/7] fixup: add initialize to DummyParser in test-http-parser-lazy-loaded --- test/parallel/test-http-parser-lazy-loaded.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-http-parser-lazy-loaded.js b/test/parallel/test-http-parser-lazy-loaded.js index 34e2fdbf559ab4..79b6ac37b3cbfe 100644 --- a/test/parallel/test-http-parser-lazy-loaded.js +++ b/test/parallel/test-http-parser-lazy-loaded.js @@ -8,7 +8,10 @@ const { getOptionValue } = require('internal/options'); // Monkey patch before requiring anything class DummyParser { constructor() { - this.test_type = DummyParser.REQUEST; + this.test_type = null; + } + initialize(type) { + this.test_type = type; } } DummyParser.REQUEST = Symbol(); @@ -25,6 +28,7 @@ const { parsers } = require('_http_common'); // Test _http_common was not loaded before monkey patching const parser = parsers.alloc(); +parser.initialize(DummyParser.REQUEST, {}); assert.strictEqual(parser instanceof DummyParser, true); assert.strictEqual(parser.test_type, DummyParser.REQUEST); From 565d515c5d0b890566453e28627f061bc3cdbd15 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Mon, 29 Apr 2019 21:13:39 +0200 Subject: [PATCH 6/7] fixup: adapt bench-parser --- benchmark/http/bench-parser.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/benchmark/http/bench-parser.js b/benchmark/http/bench-parser.js index 271ac55d007028..05776f2d8a3500 100644 --- a/benchmark/http/bench-parser.js +++ b/benchmark/http/bench-parser.js @@ -24,13 +24,14 @@ function main({ len, n }) { bench.start(); for (var i = 0; i < n; i++) { parser.execute(header, 0, header.length); - parser.initialize(REQUEST, header); + parser.initialize(REQUEST, {}); } bench.end(n); } function newParser(type) { - const parser = new HTTPParser(type); + const parser = new HTTPParser(); + parser.initialize(type, {}); parser.headers = []; From 467724561d5be50187e560a5fbdd89180bf79243 Mon Sep 17 00:00:00 2001 From: Gerhard Stoebich <18708370+Flarna@users.noreply.github.com> Date: Tue, 30 Apr 2019 12:18:09 +0200 Subject: [PATCH 7/7] fixup: move reuse decission from AsyncReset to constructor --- src/async_wrap.cc | 23 ++++++++++++++----- src/async_wrap.h | 15 +++++++++--- src/node_http_parser_impl.h | 9 ++------ test/sequential/test-async-wrap-getasyncid.js | 5 +++- 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 2dd7d808781f95..d85351ab4be3ce 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -552,6 +552,13 @@ void AsyncWrap::Initialize(Local target, } } + +AsyncWrap::AsyncWrap(Environment* env, + Local object, + ProviderType provider, + double execution_async_id) + : AsyncWrap(env, object, provider, execution_async_id, false) {} + AsyncWrap::AsyncWrap(Environment* env, Local object, ProviderType provider, @@ -566,6 +573,12 @@ AsyncWrap::AsyncWrap(Environment* env, AsyncReset(execution_async_id, silent); } +AsyncWrap::AsyncWrap(Environment* env, v8::Local object) + : BaseObject(env, object), + provider_type_(PROVIDER_NONE) { + CHECK_GE(object->InternalFieldCount(), 1); +} + AsyncWrap::~AsyncWrap() { EmitTraceEventDestroy(); @@ -601,23 +614,21 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) { } void AsyncWrap::AsyncReset(double execution_async_id, bool silent) { - AsyncReset(object(), false, execution_async_id, 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(Local resource, - bool skip_destroy, - double execution_async_id, +void AsyncWrap::AsyncReset(Local resource, double execution_async_id, bool silent) { CHECK_NE(provider_type(), PROVIDER_NONE); - if (!skip_destroy && (async_id_ != kInvalidAsyncId)) { + if (async_id_ != kInvalidAsyncId) { // 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 // before generating a new async_id. - EmitDestroy(env(), async_id_); + EmitDestroy(); } // Now we can assign a new async_id_ to this instance. diff --git a/src/async_wrap.h b/src/async_wrap.h index f709d767ea8718..20134f4a7bbfad 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -109,8 +109,11 @@ class AsyncWrap : public BaseObject { AsyncWrap(Environment* env, v8::Local object, ProviderType provider, - double execution_async_id = kInvalidAsyncId, - bool silent = false); + double execution_async_id = kInvalidAsyncId); + + // This constructor creates a reuseable instance where user is responsible + // to call set_provider_type() and AsyncReset() before use. + AsyncWrap(Environment* env, v8::Local object); ~AsyncWrap() override; @@ -160,7 +163,6 @@ class AsyncWrap : public BaseObject { inline double get_trigger_async_id() const; void AsyncReset(v8::Local resource, - bool skip_destroy, double execution_async_id = kInvalidAsyncId, bool silent = false); @@ -208,6 +210,13 @@ class AsyncWrap : public BaseObject { }; private: + friend class PromiseWrap; + + AsyncWrap(Environment* env, + v8::Local promise, + ProviderType provider, + double execution_async_id, + bool silent); ProviderType provider_type_; // Because the values may be Reset(), cannot be made const. double async_id_ = kInvalidAsyncId; diff --git a/src/node_http_parser_impl.h b/src/node_http_parser_impl.h index 96139764bd7602..a354c6fcc51eba 100644 --- a/src/node_http_parser_impl.h +++ b/src/node_http_parser_impl.h @@ -155,12 +155,7 @@ struct StringPtr { class Parser : public AsyncWrap, public StreamListener { public: Parser(Environment* env, Local wrap) - : AsyncWrap(env, - wrap, - // AsyncWrap::PROVIDER_NONE would match better here but - // there is an assert in AsyncWrap() which avoid this. - AsyncWrap::PROVIDER_HTTPINCOMINGMESSAGE, - AsyncWrap::kInvalidAsyncId, true), + : AsyncWrap(env, wrap), current_buffer_len_(0), current_buffer_data_(nullptr) { } @@ -522,7 +517,7 @@ class Parser : public AsyncWrap, public StreamListener { : AsyncWrap::PROVIDER_HTTPCLIENTREQUEST); parser->set_provider_type(provider); - parser->AsyncReset(args[1].As(), true); + parser->AsyncReset(args[1].As()); parser->Init(type); } diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index fc9dce6d4b2f99..9f5c073c9e2d06 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -152,7 +152,10 @@ if (common.hasCrypto) { // eslint-disable-line node-core/crypto-check { const { HTTPParser } = require('_http_common'); - testInitialized(new HTTPParser(), 'HTTPParser'); + const parser = new HTTPParser(); + testUninitialized(parser, 'HTTPParser'); + parser.initialize(HTTPParser.REQUEST, {}); + testInitialized(parser, 'HTTPParser'); }