Skip to content

Commit

Permalink
async_hooks: execute destroy hooks earlier
Browse files Browse the repository at this point in the history
Use a microtask to call destroy hooks in case there are a lot queued
as immediate may be scheduled late in case of long running
promise chains.

Queuing a mircrotasks in GC context is not allowed therefore an
interrupt is triggered to do this in JS context as fast as possible.

fixes: #34328
refs: #33896

PR-URL: #34342
Fixes: #34328
Refs: #33896
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Flarna authored and addaleax committed Sep 22, 2020
1 parent b3449ea commit 21be9e5
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 0 deletions.
12 changes: 12 additions & 0 deletions src/async_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,18 @@ void AsyncWrap::EmitDestroy(Environment* env, double async_id) {
env->SetUnrefImmediate(&DestroyAsyncIdsCallback);
}

// If the list gets very large empty it faster using a Microtask.
// Microtasks can't be added in GC context therefore we use an
// interrupt to get this Microtask scheduled as fast as possible.
if (env->destroy_async_id_list()->size() == 16384) {
env->RequestInterrupt([](Environment* env) {
env->isolate()->EnqueueMicrotask(
[](void* arg) {
DestroyAsyncIdsCallback(static_cast<Environment*>(arg));
}, env);
});
}

env->destroy_async_id_list()->push_back(async_id);
}

Expand Down
97 changes: 97 additions & 0 deletions test/async-hooks/test-destroy-not-blocked.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
'use strict';
// Flags: --expose_gc

const common = require('../common');
const assert = require('assert');
const tick = require('../common/tick');

const { createHook, AsyncResource } = require('async_hooks');

// Test priority of destroy hook relative to nextTick,... and
// verify a microtask is scheduled in case a lot items are queued

const resType = 'MyResource';
let activeId = -1;
createHook({
init(id, type) {
if (type === resType) {
assert.strictEqual(activeId, -1);
activeId = id;
}
},
destroy(id) {
if (activeId === id) {
activeId = -1;
}
}
}).enable();

function testNextTick() {
assert.strictEqual(activeId, -1);
const res = new AsyncResource(resType);
assert.strictEqual(activeId, res.asyncId());
res.emitDestroy();
// nextTick has higher prio than emit destroy
process.nextTick(common.mustCall(() =>
assert.strictEqual(activeId, res.asyncId()))
);
}

function testQueueMicrotask() {
assert.strictEqual(activeId, -1);
const res = new AsyncResource(resType);
assert.strictEqual(activeId, res.asyncId());
res.emitDestroy();
// queueMicrotask has higher prio than emit destroy
queueMicrotask(common.mustCall(() =>
assert.strictEqual(activeId, res.asyncId()))
);
}

function testImmediate() {
assert.strictEqual(activeId, -1);
const res = new AsyncResource(resType);
assert.strictEqual(activeId, res.asyncId());
res.emitDestroy();
setImmediate(common.mustCall(() =>
assert.strictEqual(activeId, -1))
);
}

function testPromise() {
assert.strictEqual(activeId, -1);
const res = new AsyncResource(resType);
assert.strictEqual(activeId, res.asyncId());
res.emitDestroy();
// Promise has higher prio than emit destroy
Promise.resolve().then(common.mustCall(() =>
assert.strictEqual(activeId, res.asyncId()))
);
}

async function testAwait() {
assert.strictEqual(activeId, -1);
const res = new AsyncResource(resType);
assert.strictEqual(activeId, res.asyncId());
res.emitDestroy();

for (let i = 0; i < 5000; i++) {
await Promise.resolve();
}
global.gc();
await Promise.resolve();
// Limit to trigger a microtask not yet reached
assert.strictEqual(activeId, res.asyncId());
for (let i = 0; i < 5000; i++) {
await Promise.resolve();
}
global.gc();
await Promise.resolve();
assert.strictEqual(activeId, -1);
}

testNextTick();
tick(2, testQueueMicrotask);
tick(4, testImmediate);
tick(6, testPromise);
tick(8, () => testAwait().then(common.mustCall()));

0 comments on commit 21be9e5

Please sign in to comment.