Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: fixup do not reuse HTTPParser #27477

Closed
wants to merge 7 commits into from
Closed

async_hooks: fixup do not reuse HTTPParser #27477

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
60479d4
async_hooks: fixup do not reuse HTTPParser
Flarna Apr 22, 2019
40842c6
make initialize() mandatory
Flarna Apr 29, 2019
b4c189d
fixup: revert whitespace changes
Flarna Apr 29, 2019
a8bc5f1
fixup: rename invalid_async_id to kInvalidAsyncId
Flarna Apr 29, 2019
0d790d2
fixup: add initialize to DummyParser in test-http-parser-lazy-loaded
Flarna Apr 29, 2019
565d515
fixup: adapt bench-parser
Flarna Apr 29, 2019
4677245
fixup: move reuse decission from AsyncReset to constructor
Flarna Apr 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/_http_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
127 changes: 64 additions & 63 deletions src/async_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

Expand Down Expand Up @@ -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<int64_t>(get_async_id())); \
break;
break;
NODE_ASYNC_PROVIDER_TYPES(V)
#undef V
default:
Expand All @@ -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<int64_t>(async_id)); \
break;
break;
NODE_ASYNC_PROVIDER_TYPES(V)
#undef V
default:
Expand All @@ -177,7 +177,7 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) {
class PromiseWrap : public AsyncWrap {
public:
PromiseWrap(Environment* env, Local<Object> object, bool silent)
: AsyncWrap(env, object, PROVIDER_PROMISE, -1, silent) {
: AsyncWrap(env, object, PROVIDER_PROMISE, invalid_async_id, silent) {
MakeWeak();
}

Expand Down Expand Up @@ -214,7 +214,7 @@ PromiseWrap* PromiseWrap::New(Environment* env,
void PromiseWrap::getIsChainedPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(
info.Holder()->GetInternalField(kIsChainedPromiseField));
info.Holder()->GetInternalField(kIsChainedPromiseField));
}

static PromiseWrap* extractPromiseWrap(Local<Promise> promise) {
Expand Down Expand Up @@ -293,11 +293,11 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {

Local<Object> fn_obj = args[0].As<Object>();

#define SET_HOOK_FN(name) \
#define SET_HOOK_FN(name) \
Local<Value> 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<Function>());

SET_HOOK_FN(init);
Expand Down Expand Up @@ -336,8 +336,8 @@ static void DisablePromiseHook(const FunctionCallbackInfo<Value>& 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<Isolate*>(data);
isolate->SetPromiseHook(nullptr);
Isolate* isolate = static_cast<Isolate*>(data);
isolate->SetPromiseHook(nullptr);
}, static_cast<void*>(isolate));
}

Expand All @@ -359,7 +359,7 @@ void AsyncWrap::WeakCallback(const WeakCallbackInfo<DestroyParam>& info) {
Local<Value> val;

if (!prop_bag->Get(p->env->context(), p->env->destroyed_string())
.ToLocal(&val)) {
.ToLocal(&val)) {
return;
}

Expand Down Expand Up @@ -387,7 +387,7 @@ static void RegisterDestroyHook(const FunctionCallbackInfo<Value>& args) {

void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& 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());
}
Expand All @@ -414,16 +414,21 @@ void AsyncWrap::AsyncReset(const FunctionCallbackInfo<Value>& args) {
AsyncWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
double execution_async_id =
args[0]->IsNumber() ? args[0].As<Number>()->Value() : -1;
args[0]->IsNumber() ? args[0].As<Number>()->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<Value>& args) {
CHECK(args[0]->IsNumber());
AsyncWrap::EmitDestroy(
Environment::GetCurrent(args),
args[0].As<Number>()->Value());
args[0].As<Number>()->Value());
}

Local<FunctionTemplate> AsyncWrap::GetConstructorTemplate(Environment* env) {
Expand Down Expand Up @@ -457,10 +462,10 @@ void AsyncWrap::Initialize(Local<Object> target,
PropertyAttribute ReadOnlyDontDelete =
static_cast<PropertyAttribute>(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
Expand All @@ -480,22 +485,22 @@ void AsyncWrap::Initialize(Local<Object> 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<Object> 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);
Expand All @@ -514,9 +519,9 @@ void AsyncWrap::Initialize(Local<Object> target,
FORCE_SET_TARGET_FIELD(target, "constants", constants);

Local<Object> 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);
Expand Down Expand Up @@ -547,13 +552,6 @@ void AsyncWrap::Initialize(Local<Object> target,
}
}


AsyncWrap::AsyncWrap(Environment* env,
Local<Object> object,
ProviderType provider,
double execution_async_id)
: AsyncWrap(env, object, provider, execution_async_id, false) {}

AsyncWrap::AsyncWrap(Environment* env,
Local<Object> object,
ProviderType provider,
Expand All @@ -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<int64_t>(get_async_id())); \
break;
break;
NODE_ASYNC_PROVIDER_TYPES(V)
#undef V
#undef V
default:
UNREACHABLE();
}
Expand All @@ -603,42 +600,46 @@ 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<Object> resource, double execution_async_id,
void AsyncWrap::AsyncReset(Local<Object> resource,
bool skip_destroy,
double execution_async_id,
bool silent) {
if (async_id_ != -1) {
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
// before generating a new async_id.
EmitDestroy(env(), 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<int64_t>(env()->execution_async_id())); \
data->SetInteger("triggerAsyncId", \
static_cast<int64_t>(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<int64_t>(env()->execution_async_id())); \
data->SetInteger("triggerAsyncId", \
static_cast<int64_t>(get_trigger_async_id())); \
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1( \
TRACING_CATEGORY_NODE1(async_hooks), \
#PROVIDER, static_cast<int64_t>(get_async_id()), \
"data", std::move(data)); \
} \
break;
} \
break;
NODE_ASYNC_PROVIDER_TYPES(V)
#undef V
default:
Expand Down Expand Up @@ -671,10 +672,10 @@ void AsyncWrap::EmitAsyncInit(Environment* env,
Local<Function> init_fn = env->async_hooks_init_function();

Local<Value> 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);
Expand All @@ -688,7 +689,7 @@ MaybeLocal<Value> AsyncWrap::MakeCallback(const Local<Function> 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<Value> ret = InternalMakeCallback(
env(), object(), cb, argc, argv, context);

Expand All @@ -705,7 +706,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<int64_t>(async_id_)) + ")";
std::to_string(static_cast<int64_t>(async_id_)) + ")";
}

Local<Object> AsyncWrap::GetOwner() {
Expand Down
Loading