Skip to content

inspector: process.exit should wait for inspector - #7252

Closed
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:on_exit
Closed

inspector: process.exit should wait for inspector#7252
eugeneo wants to merge 1 commit into
nodejs:masterfrom
eugeneo:on_exit

Conversation

@eugeneo

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

Waits for the inspector frontend to detach when process.exit is called. This enables the user to analyse the profiling information.

Fixes: #7088

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 9, 2016
@eugeneo

Copy link
Copy Markdown
ContributorAuthor

@ofrobots

@pavelfeldman

Copy link
Copy Markdown
Contributor

lgtm

@ofrobots

Copy link
Copy Markdown
Contributor

@bnoordhuis

Copy link
Copy Markdown
Member

LGTM

@bnoordhuis

Copy link
Copy Markdown
Member

For bonus points: factor out that magic number 32 into a constant and update PlatformInit() and WaitForInspectorDisconnect().

@eugeneo

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis I introduced a constant and rebased the CL

Comment threadsrc/node.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you name it e.g. kNumSignals or kMaxSignal? ALL_CAPS should be reserved for macros. LGTM apart from that.

@eugeneo

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis I renamed to kMaxSignal. Thanks!

@ofrobots

Copy link
Copy Markdown
Contributor

@bnoordhuis

Copy link
Copy Markdown
Member

LGTM. ppcbe-ubuntu1404 seems to be stuck but everything else is green.

@ofrobots

Copy link
Copy Markdown
Contributor

Thanks, landed as 6626919.

ofrobots pushed a commit that referenced this pull request Jun 13, 2016
Fixes: #7088
PR-URL: #7252
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@evanlucas

Copy link
Copy Markdown
Contributor

I am excluding this one from the next v6.x release since it is just a semver patch release. Once the inspector lands in v6.x, we can remove the dont-land-on-v6.x label. Thanks!

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Fixes: #7088
PR-URL: #7252
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Conflicts:
src/node.cc
@Fishrock123Fishrock123 mentioned this pull request Jul 5, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--inspect connection dies before profiler captures trace

8 participants

@eugeneo@pavelfeldman@ofrobots@bnoordhuis@evanlucas@MylesBorins@Fishrock123@nodejs-github-bot