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: report when main context is destroyed #12814

Merged
merged 0 commits into from May 5, 2017

Conversation

@eugeneo
Copy link
Contributor

commented May 3, 2017

Reimplements: #7756
Fixes: #7742

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: report when the main context is done.

@nojvek
nojvek approved these changes May 3, 2017
src/inspector_agent.cc Outdated
@@ -243,6 +243,10 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient {
return uv_hrtime() * 1.0 / NANOS_PER_MSEC;
}

void contextDestroyed(Local<Context> context) {
inspector_->contextDestroyed(context);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 3, 2017

Member

Wouldn't the symmetrical thing be to call inspector_->contextDestroyed(env_->context()) from ~NodeInspectorClient?

This comment has been minimized.

Copy link
@eugeneo

eugeneo May 3, 2017

Author Contributor

Inspector is supposed to live past the context lifetime (e.g. some tools like profiler can still work with the data it gathered).

I removed contextCreated notification from the constructor. I am trying to implement multicontext and it seems more logical if contextCreated and contextDestroyed are called from outside.

src/inspector_agent.cc Outdated
@@ -243,6 +238,17 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient {
return uv_hrtime() * 1.0 / NANOS_PER_MSEC;
}

void contextCreated(Local<Context> context, std::string name) {

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu May 4, 2017

Member

const for name?

This comment has been minimized.

Copy link
@eugeneo

eugeneo May 4, 2017

Author Contributor

Done.

@refack
refack approved these changes May 4, 2017
@jasnell
jasnell approved these changes May 4, 2017
@ak239
ak239 approved these changes May 4, 2017

@eugeneo eugeneo closed this May 5, 2017

@eugeneo eugeneo deleted the eugeneo:contextDestroyed branch May 5, 2017

@eugeneo eugeneo merged commit 15e160e into nodejs:master May 5, 2017

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
inspector: report when main context is destroyed
PR-URL: nodejs#12814
Reimplements: nodejs#7756
Fixes: nodejs#7742
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@ianguerin

This comment has been minimized.

Copy link

commented Nov 22, 2017

@eugeneo, this issue is still happening when I'm debugging with node@8.4.0 and Chrome (macOS) v62.0.3202.94. I did not see it mentioned in the release proposal for 8.0.0, but this thread led me to believe it would be fixed in 8.x.x?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 22, 2017

@ianguerin Try the latest 8.x release, v8.9.1.

@ianguerin

This comment has been minimized.

Copy link

commented Nov 22, 2017

Thank you @bnoordhuis, having some issues with node-gyp after bumping to 8.9.1. I'll update when I'm able to get things up and running again

@ianguerin

This comment has been minimized.

Copy link

commented Nov 27, 2017

@bnoordhuis, appears to be the exact same issue, even with node@8.9.1

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

@ianguerin Can you file a new issue? Please include steps to reproduce. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.