-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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,lib: minor --debug-brk cleanup #6599
Conversation
eb95df5
to
e74840a
Compare
Is this related to #2546? |
We believe this is a genuine bug, but we uncovered it while working on On Thu, May 5, 2016 at 2:00 PM Colin Ihrig notifications@github.com wrote:
|
LGTM but it would be even better if the property was deleted again before it's visible to user code. |
e74840a
to
35c3703
Compare
@bnoordhuis That's a good idea. Please take another look. New CI: https://ci.nodejs.org/job/node-test-pull-request/2525/ |
LGTM |
1 similar comment
LGTM |
Weird failure on the fedora23 bot, by the way: https://ci.nodejs.org/job/node-test-commit-linux/3256/nodes=fedora23/console
I don't know what's causing it but it looks unrelated. |
Minor cleanup of how --debug-brk works: * We no longer need to use command line flags to expose the debug object. * Do not depend on the existence of global.v8debug as a mechanism to determine if --debug-brk was specified. * We no longer need to set a dummy listener with --debug-brk. PR-URL: nodejs#6599 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
35c3703
to
4d4cfb2
Compare
Thanks. Landed as 4d4cfb2. |
Minor cleanup of how --debug-brk works: * We no longer need to use command line flags to expose the debug object. * Do not depend on the existence of global.v8debug as a mechanism to determine if --debug-brk was specified. * We no longer need to set a dummy listener with --debug-brk. PR-URL: #6599 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
@ofrobots lts? |
I don't see a very strong benefit, or much harm from back-porting this to 4.x LTS. This should be back-portable, modulo the |
@ofrobots I'm moving this to don't land for now, please let me know if you would like to send a PR |
How am I supposed to programatically detect whether I'm in debug mode now if you don't expose global.v8debug? I tried checking process._debugWaitConnect but that is undefined too even when I pass --debug-brk=12345. |
You could check with |
@bnoordhuis Because if it's debugging, I need to change the argv for each new forked process so they don't have a debugging port clash with existing processes. |
Are you using |
Yeah, I am using that as I don't want processes to share ports. I don't know why the port shuffle wasn't extended, the principle is the same with child_process. |
In fact, is there any reason why i should use child_process instead of cluster? If the only difference is that cluster can share TCP ports, child_process seems to be redundant at this point. Kind of makes you wonder why they created a whole new core module for cluster. |
With all due respect, but if you think that is the only difference between the two modules, you need to read the documentation (or their sources) more closely. |
From my reading of the docs that is basically the only difference. Also, this StackOverflow answer agrees with me. Can you point out any other differences? |
Maybe you are simply being imprecise with language? You say 'module' but going by the link you posted, you really mean |
Checklist
Affected core subsystem(s)
src,lib
Description of change
Minor cleanup of how --debug-brk works:
object.
determine if --debug-brk was specified.
We may overwrite the debuglistener with the dummy listener when --expose-debug-as=v8debug
was otherwise present on the command line.
This in anticipation of an upcoming PR with v8-inspector support.
R=@bnoordhuis
/cc @eugeneo