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

inspector: no async tracking for promises #17118

Closed
wants to merge 2 commits into from
Closed

inspector: no async tracking for promises #17118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fb6f70
inspector: no async tracking for promises
addaleax Nov 18, 2017
d6a5991
[squash] rename promises → promiseIds
addaleax Nov 18, 2017
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
13 changes: 12 additions & 1 deletion lib/internal/inspector_async_hook.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,33 @@ const hook = createHook({
// in https://github.com/nodejs/node/pull/13870#discussion_r124515293,
// this should be fine as long as we call asyncTaskCanceled() too.
const recurring = true;
inspector.asyncTaskScheduled(type, asyncId, recurring);
if (type === 'PROMISE')
this.promiseIds.add(asyncId);
else
inspector.asyncTaskScheduled(type, asyncId, recurring);
},

before(asyncId) {
if (this.promiseIds.has(asyncId))
return;
inspector.asyncTaskStarted(asyncId);
},

after(asyncId) {
if (this.promiseIds.has(asyncId))
return;
inspector.asyncTaskFinished(asyncId);
},

destroy(asyncId) {
if (this.promiseIds.has(asyncId))
return this.promiseIds.delete(asyncId);
inspector.asyncTaskCanceled(asyncId);
},
});

hook.promiseIds = new Set();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this super implementation dependent? I don't think we are testing or supporting what this actually refers to. Is there any advantage of this over const promiseIds = new Set();?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don’t want to guarantee this being set to the object passed in (which I think we should guarantee, since it’s the most obvious thing), we should set this to undefined or so. Otherwise other people are going to come to rely on it as well.

Copy link
Member

@AndreasMadsen AndreasMadsen Nov 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was confusing to me because I expected this to refer to the options object. That is how this works in methods. However, because we reassign the values internally the this object becomes something else.

To me, this would be the "obviouse thing":

 const hook = createHook({
  promiseIds: new Set(),
  init(asyncId, type, triggerAsyncId, resource) {
    console.log(this.promiseIds);
  }
})

However, that doesn't work.

Anyway, I'm fine with either adding tests and documentation for this or having no context. I suppose I would prefer the latter, but it is not a big deal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the context be the object passed into createHooks would bring symmetry with Proxy handlers, but even in the spec community that was a controversial choice (difficulty to extend the range of handlers w/o breaking web compatibility).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu Yeah, I feel doing that brings in much more complexity than it removes. const promiseIds = new Set(); is such a simple solution.


function enable() {
if (config.bits < 64) {
// V8 Inspector stores task ids as (void*) pointers.
Expand Down
2 changes: 1 addition & 1 deletion test/sequential/test-inspector-async-stack-traces-promise-then.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function debuggerPausedAt(msg, functionName, previousTickLocation) {
`${Object.keys(msg.params)} contains "asyncStackTrace" property`);

assert.strictEqual(msg.params.callFrames[0].functionName, functionName);
assert.strictEqual(msg.params.asyncStackTrace.description, 'PROMISE');
assert.strictEqual(msg.params.asyncStackTrace.description, 'Promise.resolve');

const frameLocations = msg.params.asyncStackTrace.callFrames.map(
(frame) => `${frame.functionName}:${frame.lineNumber}`);
Expand Down