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.

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 promise not destroyed #14446

Closed
wengeezhang opened this issue Jul 24, 2017 · 6 comments
Closed

async hooks promise not destroyed #14446

wengeezhang opened this issue Jul 24, 2017 · 6 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem. question Issues that look for answers.

Comments

@wengeezhang
Copy link

wengeezhang commented Jul 24, 2017

  • Version: node -v 8.2.1
  • Platform: window 7 64-bit
  • Subsystem:

my code is like follows:

const async_hooks = require('async_hooks');

function init(id, type, triggerId, handle) {
    process._rawDebug(`id: ${id}, type: ${type}, triggerId: ${triggerId}, 
                                        handle.promise: ${handle.promise}, 
                                        handle.parentid: ${handle.parentId}`);
}


function before(id) { 
    process._rawDebug('before', id, async_hooks.executionAsyncId());
 }
function after(id) { 
    process._rawDebug('after', id, async_hooks.executionAsyncId());
 }
function destroy(id) {
    process._rawDebug('destroy', id);
}

async_hooks.createHook({init, before, after, destroy}).enable();
debugger;

const parent_promise = new Promise((resolve, reject) => {resolve(5);}); 
const promise = parent_promise.then((val) => {return val;});

the output is:

id: 2, type: PROMISE, triggerId: 1
id: 3, type: PROMISE, triggerId: 2
before 3 3
after 3 3

why the Promise async resources' destroy callback not triggered?

@mscdex mscdex added the async_hooks Issues and PRs related to the async hooks subsystem. label Jul 24, 2017
@AndreasMadsen
Copy link
Member

Promises are only destroyed by the V8 garbage collector. If you give the process some time it should eventually destroy them.

@AndreasMadsen AndreasMadsen added the question Issues that look for answers. label Jul 24, 2017
@Fishrock123
Copy link
Contributor

Yes, exactly what @AndreasMadsen said.

(Generally avoid holding onto any resource of handle for an indeterminate amount of time.)

@wengeezhang
Copy link
Author

wengeezhang commented Jul 25, 2017

const async_hooks = require('async_hooks');
let map = new Map();

function init(asyncId, type, triggerId, resource) {
    let emptyObj = {};
    Error.captureStackTrace(emptyObj, init);
    map.set(asyncId,emptyObj.stack)
}


function before(asyncId) { 
    global.longStack = map.get(asyncId);
 }
function after(asyncId) { 

 }
function destroy(asyncId) {
  map.delete(asyncId);
}

async_hooks.createHook({init, before, after, destroy}).enable();
debugger;

const parent_promise = new Promise((resolve, reject) => {resolve(5);}); 
const promise = parent_promise.then((val) => {return val;});

i want to implement a 'long stack trace' using async hooks like this:

  • store stack in a map when async resource's initiated

  • restore the stack in 'before' callback.

how can i delete the stack in the map?

(not using weakmap,because

Note: In some cases the resource object is reused for performance reasons, it is thus not safe to use it as a key in a WeakMap or add properties to it.

in nodejs api doc)

@Fishrock123

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Jul 25, 2017

In terms of destroy your implementation is fine. You are right that the promises aren't immediately destroyed and thus the stack traces are also not immediately destroyed. However, this is fine, what is important is that they are eventually destroyed, otherwise you have a memory leak.

PS: You should be aware that before and after can be called multiple times in the same tick. (e.g. before(1), before(2), after(2), after(1)), thus global.longStack should be a stack and not a single value.

You can look at my long stack trace (https://github.com/AndreasMadsen/trace/blob/master/trace.js) for more inspiration.

@wengeezhang
Copy link
Author

@AndreasMadsen Thanks for your info:

PS: You should be aware that before and after can be called multiple times in the same tick. (e.g. before(1), before(2), after(2), after(1)), thus global.longStack should be a stack and not a single value.

i've inspected your implementation and gained much.
However,when i use your implementation to test

const parent_promise = new Promise((resolve, reject) => {resolve(5);}); 
const promise = parent_promise.then((val) => {return val;});

i got the same result:
after half an hour,the traces map in your implementation still has 'the Promise' stack. If the site is busy(more and more requests come),then the 'traces' will eat more and more memeroy

@wengeezhang
Copy link
Author

my worries in this issue do not exist!!!

thanks @AndreasMadsen
i've confirmed that promise is eventually destroyed.
If it's not destroyed(maybe a long time,several hours,or several days),it's really ok.

The reason is:

  • promises not to many,GC think it's not time to trigger a collection(expensive)
  • when there're to many,GC will trigger collection(you can make new more promises to verify this)

AndreasMadsen added a commit to AndreasMadsen/node that referenced this issue Oct 12, 2018
While it doesn't make any difference now. In the future PromiseHooks
could be refactored to provide an asyncId instead of the promise object.
That would make escape analysis on promises possible.

Escape analysis on promises could lead to a more efficient destroy hook,
if provide by PromiseHooks as well. But at the very least would allow
the destroy hook to be emitted earlier. The destroy hook not being
emitted on promises frequent enough is a known and reported issue.
See nodejs#14446 and
Jeff-Lewis/cls-hooked#11.

While all this is speculation for now, it all depends on the promise
object not being a part of the PromiseWrap resource object.

Ref: nodejs#14446
Ref: nodejs/diagnostics#188
gdams pushed a commit that referenced this issue Oct 15, 2018
While it doesn't make any difference now. In the future PromiseHooks
could be refactored to provide an asyncId instead of the promise object.
That would make escape analysis on promises possible.

Escape analysis on promises could lead to a more efficient destroy hook,
if provide by PromiseHooks as well. But at the very least would allow
the destroy hook to be emitted earlier. The destroy hook not being
emitted on promises frequent enough is a known and reported issue.
See #14446 and
Jeff-Lewis/cls-hooked#11.

While all this is speculation for now, it all depends on the promise
object not being a part of the PromiseWrap resource object.

Ref: #14446
Ref: nodejs/diagnostics#188

PR-URL: #23443
Refs: #14446
Refs: nodejs/diagnostics#188
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

4 participants