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: check if connected before waiting #10094

Merged
merged 0 commits into from
Dec 6, 2016
Merged

inspector: check if connected before waiting #10094

merged 0 commits into from
Dec 6, 2016

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Dec 2, 2016

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

inspector: a check was added

Description of change

This change adds a check to see if there's a frontend connection before waiting for a disconnect.

Fixes: #10093

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol labels Dec 2, 2016

let child = null;

function DidNotShutdown() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this didNotShutdown() please.

common.fail('Did not shut down');
}

const timeout = setTimeout(DidNotShutdown, 60 * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a long timeout for a test.

Copy link
Member

Choose a reason for hiding this comment

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

I doubt it will ever trigger during a normal run because the test runner is going to kill the test after 5 or 10 seconds. It's probably easiest to simply scrap this code.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

A few nits on the test, but LGTM.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion. Please fix the style issue if you want to keep the timeout.

common.fail('Did not shut down');
}

const timeout = setTimeout(DidNotShutdown, 60 * 1000);
Copy link
Member

Choose a reason for hiding this comment

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

I doubt it will ever trigger during a normal run because the test runner is going to kill the test after 5 or 10 seconds. It's probably easiest to simply scrap this code.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 6, 2016

Thank you for the reviews, I've updated the test case.


process.on('exit', exitHandler);
process.on('SIGINT', exitHandler);
process.on('SIGTERM', exitHandler);
Copy link
Member

Choose a reason for hiding this comment

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

Installing listeners for SIGINT and SIGTERM will override the default behavior of terminating when the signal is received. Call process.exit() in the callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. It is now using exit code 1 to indicate error. I also removed the 'exit' handler.


const child = spawn(process.execPath,
[ '--inspect', 'no-such-script.js' ],
{ 'stdio': 'inherit' });
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the arguments line up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 6, 2016

Seems like https://ci.nodejs.org/job/node-test-pull-request/5269/ is all green but results are not reported back to GitHub for some reason.

@eugeneo eugeneo closed this Dec 6, 2016
@eugeneo eugeneo merged commit 2f3865d into nodejs:master Dec 6, 2016
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 6, 2016

Landed as 2f3865d

@eugeneo eugeneo deleted the should_quit branch December 6, 2016 22:35
Fishrock123 pushed a commit that referenced this pull request Dec 13, 2016
Fixes: #10093
PR-URL: #10094
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
Fixes: #10093
PR-URL: #10094
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
Fixes: #10093
PR-URL: #10094
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
Fixes: #10093
PR-URL: #10094
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to 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++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inspector: Node waiting for frontend disconnect when unable to find the script file
6 participants