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

bootstrap: adds exception handling for profiler bootstrap #29552

Conversation

shobhitchittora
Copy link
Contributor

@shobhitchittora shobhitchittora commented Sep 14, 2019

The bootstrapping of profiler failed the script evaluation when inspector is disabled. Adding a try-catch block to handle that and emit a warning.

Fixes: #29542

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@shobhitchittora shobhitchittora force-pushed the profiler-bootstrap-exception-handling branch from bb08f04 to e9e049e Compare September 14, 2019 00:32
@shobhitchittora shobhitchittora changed the title internal/bootstrap: adds exception handling for profiler bootstrap bootstrap: adds exception handling for profiler bootstrap Sep 14, 2019
@shobhitchittora
Copy link
Contributor Author

@addaleax The warning message can be better here. Any suggestions? Any straightforward way to write a test for this. Thinking of emitting the warning from child to parent and then asserting that.

@devsnek
Copy link
Member

devsnek commented Sep 14, 2019

how about "The inspector is disabled, coverage could not be collected"

@devsnek devsnek added coverage Issues and PRs related to native coverage support. inspector Issues and PRs related to the V8 inspector protocol labels Sep 14, 2019
@addaleax
Copy link
Member

@addaleax The warning message can be better here. Any suggestions?

I think something along @devsnek’s suggestion is fine.

Any straightforward way to write a test for this. Thinking of emitting the warning from child to parent and then asserting that.

Yeah, spawning a child process that emits this warning and then checking its output would be the easiest way to test this 👍

@shobhitchittora
Copy link
Contributor Author

shobhitchittora commented Sep 15, 2019

Both
./node ./test/parallel/test-coverage-with-inspector-disabled.js
and
python tools/test.py test/parallel/test-coverage-with-inspector-disabled.js
work fine for me on local, but the test is fails in CI. I might be missing something here. Is the child spawned correctly in the test added?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Both
./node ./test/parallel/test-coverage-with-inspector-disabled.js
and
python tools/test.py test/parallel/test-coverage-with-inspector-disabled.js
work fine for me on local, but the test is fails in CI. I might be missing something here. Is the child spawned correctly in the test added?

I might be wrong, but is there a chance it’s working for you locally because you locally compiled a version of Node.js that has the inspector disabled, but Travis CI builds the default version with inspector enabled?

I think it might make sense to add something like if (process.features.inspector) common.skip('Inspector enabled'); or something like it to the beginning of the test?

test/fixtures/v8-coverage/subprocess.js Outdated Show resolved Hide resolved
test/parallel/test-coverage-with-inspector-disabled.js Outdated Show resolved Hide resolved
test/parallel/test-coverage-with-inspector-disabled.js Outdated Show resolved Hide resolved
@shobhitchittora
Copy link
Contributor Author

@addaleax Can this be closed and merged now?

@addaleax
Copy link
Member

@shobhitchittora Yeah, once CI is green with the latest changes this should be good to go 👍

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 18, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 23, 2019

@shobhitchittora Can you rebase to get rid of the conflict?

@Trott Trott force-pushed the profiler-bootstrap-exception-handling branch from 0700615 to bbb315d Compare September 23, 2019 02:17
@Trott
Copy link
Member

Trott commented Sep 23, 2019

@shobhitchittora Can you rebase to get rid of the conflict?

Never mind. I did it myself. At least one re-review would be good. @addaleax @joyeecheung

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Sep 23, 2019
Add exception handling for the case when profile is
not bootstrapped when coverage is enabled.

Fixes: #29542

PR-URL: #29552
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Member

Landed in fdd5d4a 🎉 Thanks for the PR!

@addaleax addaleax closed this Sep 23, 2019
targos pushed a commit that referenced this pull request Sep 23, 2019
Add exception handling for the case when profile is
not bootstrapped when coverage is enabled.

Fixes: #29542

PR-URL: #29552
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@shobhitchittora
Copy link
Contributor Author

Thanks all for merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. coverage Issues and PRs related to native coverage support. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NODE_V8_COVERAGE does not fail gracefully when inspector is disabled
7 participants