-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
src,lib: minor --debug-brk cleanup #6599
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
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