Skip to content

Commit

Permalink
async_hooks: avoid double-destroy HTTPParser
Browse files Browse the repository at this point in the history
Avoid that destroy hook is invoked twice - once via `Parser::Free()`
and once again via `Parser::Reinitialize()` by clearing the async_id
in `EmitDestroy()`.

Partial backport of #27477, a full
backport would require also #25094
which has a dont-land-on-v10.x label on it.

Fixes: #26961

Backport-PR-URL: #27986
PR-URL: #27477
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
Flarna authored and BethGriggs committed Jun 28, 2019
1 parent 8516da1 commit 8a3ffca
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 18 deletions.
31 changes: 18 additions & 13 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,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, kInvalidAsyncId, silent) {
MakeWeak();
}

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

void AsyncWrap::GetAsyncId(const FunctionCallbackInfo<Value>& args) {
AsyncWrap* wrap;
args.GetReturnValue().Set(-1);
args.GetReturnValue().Set(kInvalidAsyncId);
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
args.GetReturnValue().Set(wrap->get_async_id());
}
Expand All @@ -409,10 +409,15 @@ 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() : kInvalidAsyncId;
wrap->AsyncReset(execution_async_id);
}

void AsyncWrap::EmitDestroy() {
AsyncWrap::EmitDestroy(env(), async_id_);
// Ensure no double destroy is emitted via AsyncReset().
async_id_ = kInvalidAsyncId;
}

void AsyncWrap::QueueDestroyAsyncId(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsNumber());
Expand Down Expand Up @@ -474,7 +479,7 @@ 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 kInvalidAsyncId.
FORCE_SET_TARGET_FIELD(target,
"async_id_fields",
env->async_hooks()->async_id_fields().GetJSArray());
Expand Down Expand Up @@ -558,15 +563,15 @@ AsyncWrap::AsyncWrap(Environment* env,
CHECK_NE(provider, PROVIDER_NONE);
CHECK_GE(object->InternalFieldCount(), 1);

async_id_ = -1;
async_id_ = kInvalidAsyncId;
// Use AsyncReset() call to execute the init() callbacks.
AsyncReset(execution_async_id, silent);
}


AsyncWrap::~AsyncWrap() {
EmitTraceEventDestroy();
EmitDestroy(env(), get_async_id());
EmitDestroy();
}

void AsyncWrap::EmitTraceEventDestroy() {
Expand Down Expand Up @@ -602,16 +607,16 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
// 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) {
if (async_id_ != -1) {
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.
async_id_ =
execution_async_id == -1 ? env()->new_async_id() : execution_async_id;
async_id_ = execution_async_id == kInvalidAsyncId ? env()->new_async_id()
: execution_async_id;
trigger_async_id_ = env()->get_default_trigger_async_id();

switch (provider_type()) {
Expand Down Expand Up @@ -693,7 +698,7 @@ async_id AsyncHooksGetExecutionAsyncId(Isolate* isolate) {
// Environment::GetCurrent() allocates a Local<> handle.
HandleScope handle_scope(isolate);
Environment* env = Environment::GetCurrent(isolate);
if (env == nullptr) return -1;
if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
return env->execution_async_id();
}

Expand All @@ -702,7 +707,7 @@ async_id AsyncHooksGetTriggerAsyncId(Isolate* isolate) {
// Environment::GetCurrent() allocates a Local<> handle.
HandleScope handle_scope(isolate);
Environment* env = Environment::GetCurrent(isolate);
if (env == nullptr) return -1;
if (env == nullptr) return AsyncWrap::kInvalidAsyncId;
return env->trigger_async_id();
}

Expand All @@ -727,7 +732,7 @@ async_context EmitAsyncInit(Isolate* isolate,
CHECK_NOT_NULL(env);

// Initialize async context struct
if (trigger_async_id == -1)
if (trigger_async_id == AsyncWrap::kInvalidAsyncId)
trigger_async_id = env->get_default_trigger_async_id();

async_context context = {
Expand Down
11 changes: 8 additions & 3 deletions src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,12 @@ class AsyncWrap : public BaseObject {
AsyncWrap(Environment* env,
v8::Local<v8::Object> object,
ProviderType provider,
double execution_async_id = -1);
double execution_async_id = kInvalidAsyncId);

virtual ~AsyncWrap();

static constexpr double kInvalidAsyncId = -1;

static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
Environment* env);

Expand All @@ -137,6 +139,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();
Expand All @@ -149,7 +153,8 @@ class AsyncWrap : public BaseObject {

inline double get_trigger_async_id() const;

void AsyncReset(double execution_async_id = -1, bool silent = false);
void AsyncReset(double execution_async_id = kInvalidAsyncId,
bool silent = false);

// Only call these within a valid HandleScope.
v8::MaybeLocal<v8::Value> MakeCallback(const v8::Local<v8::Function> cb,
Expand Down Expand Up @@ -202,7 +207,7 @@ class AsyncWrap : public BaseObject {
inline AsyncWrap();
const ProviderType provider_type_;
// Because the values may be Reset(), cannot be made const.
double async_id_ = -1;
double async_id_ = kInvalidAsyncId;
double trigger_async_id_;
};

Expand Down
3 changes: 1 addition & 2 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -383,14 +383,13 @@ class Parser : public AsyncWrap, public StreamListener {


static void Free(const FunctionCallbackInfo<Value>& 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();
}


Expand Down
53 changes: 53 additions & 0 deletions test/parallel/test-async-hooks-http-parser-nodouble-destroy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { createHook } = 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 httpParsers = [];
const dupDestroys = [];
const destroyed = [];

createHook({
init(asyncId, type, triggerAsyncId, resource) {
if (type === 'HTTPPARSER') {
httpParsers.push(asyncId);
}
},
destroy(asyncId) {
if (destroyed.includes(asyncId)) {
dupDestroys.push(asyncId);
} else {
destroyed.push(asyncId);
}
}
}).enable();

const server = http.createServer((req, res) => {
res.end();
});

server.listen(common.mustCall(() => {
http.get({ port: server.address().port }, common.mustCall(() => {
server.close(common.mustCall(() => {
server.listen(common.mustCall(() => {
http.get({ port: server.address().port }, common.mustCall(() => {
server.close(common.mustCall(() => {
setTimeout(common.mustCall(verify), 200);
}));
}));
}));
}));
}));
}));

function verify() {
assert.strictEqual(httpParsers.length, 4);

assert.strictEqual(dupDestroys.length, 0);
httpParsers.forEach((id) => assert.ok(destroyed.includes(id)));
}

0 comments on commit 8a3ffca

Please sign in to comment.