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

src: make SIGPROF message a real warning #12709

Merged
merged 1 commit into from
May 11, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 27, 2017

This commit replaces a fprintf() with a call to ProcessEmitWarning().

R= @Fishrock123

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Apr 27, 2017
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.

It would be good to have a test if that’s possible (it might be?) but LGTM either way

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Agree with @addaleax that a test would be good if possible but otherwise LGTM

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Test and CI, but otherwise LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 28, 2017

So it looks like the inspector is always started by default, making the surrounding env->inspector_agent()->IsStarted() check always true. That means that the warning will always be emitted if process.on('SIGPROF', ...) is used in the application. That seems wrong to me.

@Fishrock123
Copy link
Contributor

@cjihrig That sounds incorrect to me too. That is why I was wondering in the issue why the event emitted even though we were not using the inspector...

@sam-github
Copy link
Contributor

We always attempt to start the inspector, but it doesn't necessarily succeed. I follow

node/src/node.cc

Line 3986 in 6bcf65d

debugger_running = v8_platform.StartInspector(env, path, debug_options);
down the bottom I get lost in the weeds, but it does appear that some low-level error conditions can cause the inspector to fail to start.

Also, building without the inspector cause it not to start :-)

Since SIGPROF is for the profiling timer (see setitimer(2)), maybe always warning about it is a good idea? On the one hand, it removes a signal from the useable set, but on the other, it sucks to write code that later fails to work when its run under the debugger.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 5, 2017

Right, we always attempt to start the inspector unless Node is built without it. If others are ok with this behavior, I can add a simple regression test.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 10, 2017

This commit replaces a fprintf() with a call to
ProcessEmitWarning().

Refs: nodejs#12706
PR-URL: nodejs#12709
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@cjihrig cjihrig merged commit b6001a2 into nodejs:master May 11, 2017
@cjihrig cjihrig deleted the warn branch May 11, 2017 19:38
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
This commit replaces a fprintf() with a call to
ProcessEmitWarning().

Refs: nodejs#12706
PR-URL: nodejs#12709
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
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++. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants