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

inspector: no async tracking for promises #17118

Closed

Conversation

addaleax
Copy link
Member

Promise instances are already tracked by V8 itself.
This fixes sequential/test-inspector-async-stack-traces-promise-then
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

Ref: https://chromium-review.googlesource.com/c/v8/v8/+/707058 (fyi @ak239)
Fixes: #17017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

`Promise` instances are already tracked by V8 itself.
This fixes `sequential/test-inspector-async-stack-traces-promise-then`
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

Ref: https://chromium-review.googlesource.com/c/v8/v8/+/707058
Fixes: nodejs#17017
@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. inspector Issues and PRs related to the V8 inspector protocol promises Issues and PRs related to ECMAScript promises. v8 engine Issues and PRs related to the V8 dependency. labels Nov 18, 2017
inspector.asyncTaskCanceled(asyncId);
},
});

hook.promises = 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.

I know promises are properly removed from the Set when it is destroyed but from a performance standpoint would a WeakSet be more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimothyGu This stores the async ids, not the promise objects themselves … do you think I should rename this? 😄

Copy link
Member

Choose a reason for hiding this comment

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

ohh, oops. Yeah renaming would be great, although just because I made this mistake doesn’t mean any other person would.

@addaleax
Copy link
Member Author

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 19, 2017
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

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.

@addaleax
Copy link
Member Author

Landed in c4a5de5

@addaleax addaleax closed this Nov 21, 2017
@addaleax addaleax deleted the inspector-async-no-promise-tracking branch November 21, 2017 12:58
addaleax added a commit that referenced this pull request Nov 21, 2017
`Promise` instances are already tracked by V8 itself.
This fixes `sequential/test-inspector-async-stack-traces-promise-then`
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

PR-URL: #17118
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/707058
Fixes: #17017
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
`Promise` instances are already tracked by V8 itself.
This fixes `sequential/test-inspector-async-stack-traces-promise-then`
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

PR-URL: #17118
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/707058
Fixes: #17017
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
`Promise` instances are already tracked by V8 itself.
This fixes `sequential/test-inspector-async-stack-traces-promise-then`
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

PR-URL: #17118
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/707058
Fixes: #17017
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
`Promise` instances are already tracked by V8 itself.
This fixes `sequential/test-inspector-async-stack-traces-promise-then`
in debug mode (it previously crashed because our tracking
and the V8 tracking were not properly nested).

PR-URL: #17118
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/707058
Fixes: #17017
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
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. inspector Issues and PRs related to the V8 inspector protocol promises Issues and PRs related to ECMAScript promises. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug build assertion failure in V8 for sequential/test-inspector-async-stack-traces-promise-then
9 participants