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,lib: minor --debug-brk cleanup #6599

Merged
merged 1 commit into from
May 7, 2016

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented May 5, 2016

Checklist
  • tests and code linting passes
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

src,lib

Description of change

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 may overwrite the debug
    listener with the dummy listener when --expose-debug-as=v8debug
    was otherwise present on the command line.
  • We no longer need to set a dummy listener with --debug-brk.

This in anticipation of an upcoming PR with v8-inspector support.

R=@bnoordhuis
/cc @eugeneo

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 5, 2016
@ofrobots ofrobots force-pushed the debug-wait-connect branch 2 times, most recently from eb95df5 to e74840a Compare May 5, 2016 17:04
@ofrobots
Copy link
Contributor Author

ofrobots commented May 5, 2016

@cjihrig
Copy link
Contributor

cjihrig commented May 5, 2016

Is this related to #2546?

@eugeneo
Copy link
Contributor

eugeneo commented May 5, 2016

We believe this is a genuine bug, but we uncovered it while working on
inspector integration.

On Thu, May 5, 2016 at 2:00 PM Colin Ihrig [email protected] wrote:

Is this related to #2546 #2546?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6599 (comment)

@bnoordhuis
Copy link
Member

LGTM but it would be even better if the property was deleted again before it's visible to user code.

@ofrobots
Copy link
Contributor Author

ofrobots commented May 6, 2016

@bnoordhuis That's a good idea. Please take another look. New CI: https://ci.nodejs.org/job/node-test-pull-request/2525/

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2016

LGTM

1 similar comment
@bnoordhuis
Copy link
Member

LGTM

@bnoordhuis
Copy link
Member

Weird failure on the fedora23 bot, by the way: https://ci.nodejs.org/job/node-test-commit-linux/3256/nodes=fedora23/console

make[2]: Leaving directory '/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora23/out'
ln -fs out/Release/node node
/bin/sh: line 1: ./node: No such file or directory
Makefile:148: recipe for target 'test/addons/.buildstamp' failed
make[1]: *** [test/addons/.buildstamp] Error 1
make[1]: Leaving directory '/home/iojs/build/workspace/node-test-commit-linux/nodes/fedora23'
Makefile:285: recipe for target 'run-ci' failed
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

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 <[email protected]>
Reviewed-By: cjihrig - Colin Ihrig <[email protected]>
@ofrobots ofrobots merged commit 4d4cfb2 into nodejs:master May 7, 2016
@ofrobots
Copy link
Contributor Author

ofrobots commented May 7, 2016

Thanks. Landed as 4d4cfb2.

@ofrobots ofrobots deleted the debug-wait-connect branch May 7, 2016 03:31
evanlucas pushed a commit that referenced this pull request May 17, 2016
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 <[email protected]>
Reviewed-By: cjihrig - Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

@ofrobots lts?

@ofrobots
Copy link
Contributor Author

ofrobots commented Jun 2, 2016

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 lib/module.js refactoring since 4.x. LMK if you want a PR w/ the backport.

@MylesBorins
Copy link
Contributor

@ofrobots I'm moving this to don't land for now, please let me know if you would like to send a PR

@jez9999
Copy link

jez9999 commented Oct 12, 2016

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.

@bnoordhuis
Copy link
Member

You could check with process.execArgv.some(s => /^--debug-brk=/.test(s)). I'm curious though, why does it matter to your code if debugging is on or not?

@jez9999
Copy link

jez9999 commented Oct 12, 2016

@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.

@bnoordhuis
Copy link
Member

Are you using child_process.fork()? There was talk recently of extending the port shuffle mechanism in cluster.fork() to child_process.fork().

@jez9999
Copy link

jez9999 commented Oct 12, 2016

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.

@jez9999
Copy link

jez9999 commented Oct 12, 2016

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.

@bnoordhuis
Copy link
Member

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.

@jez9999
Copy link

jez9999 commented Oct 12, 2016

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?

@bnoordhuis
Copy link
Member

Maybe you are simply being imprecise with language? You say 'module' but going by the link you posted, you really mean cluster.fork() vs. child_process.fork(). Is that correct?

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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants