Skip to content

Commit

Permalink
async_wrap: remove erroneous destroy list clear()
Browse files Browse the repository at this point in the history
Remove a `.clear()` call on the list of destroy ids that may
inadvertently swallow `destroy` events.

The list is already cleared earlier in the `DestroyIdsCb` function,
so usually this was a no-op; but when the garbage collection or
its equivalent was active during a `destroy` hook itself, it was
possible that `destroy` hooks were scheduled but cleared before the
next event loop iteration in which they would have been emitted.

Ref: #13286
  • Loading branch information
addaleax committed May 31, 2017
1 parent ae6c704 commit a052d4a
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
2 changes: 0 additions & 2 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,6 @@ static void DestroyIdsCb(uv_idle_t* handle) {
FatalException(env->isolate(), try_catch);
}
}

env->destroy_ids_list()->clear();
}


Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-async-hooks-close-during-destroy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';
// Test that async ids that are added to the destroy queue while running a
// `destroy` callback are handled correctly.

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

const initCalls = new Set();
let destroyResCallCount = 0;
let res2;

async_hooks.createHook({
init: common.mustCallAtLeast((id, provider, triggerId) => {
if (provider === 'foobar')
initCalls.add(id);
}, 2),
destroy: common.mustCallAtLeast((id) => {
if (!initCalls.has(id)) return;

switch (destroyResCallCount++) {
case 0:
// Trigger the second `destroy` call.
res2.emitDestroy();
break;
case 2:
assert.fail('More than 2 destroy() invocations');
break;
}
}, 2)
}).enable();

const res1 = new async_hooks.AsyncResource('foobar');
res2 = new async_hooks.AsyncResource('foobar');
res1.emitDestroy();

process.on('exit', () => assert.strictEqual(destroyResCallCount, 2));

// Keep the event loop alive for a small bit so that the `destroy` callback
// can run. This can be removed once https://github.com/nodejs/node/issues/13262
// is resolved.
setTimeout(() => {}, 100);

0 comments on commit a052d4a

Please sign in to comment.