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

debugger: make listen address configurable #3316

Merged

Conversation

bnoordhuis
Copy link
Member

@indutny
Copy link
Member

indutny commented Oct 10, 2015

The change is good, but I'm not sure about the CLI arguments usage. Why not introduce --debug-host?

@indutny
Copy link
Member

indutny commented Oct 10, 2015

This will chop off many of the changes in this PR.

@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 10, 2015
@bnoordhuis
Copy link
Member Author

Why not introduce --debug-host?

Because I hate having to type two arguments when one will do.

@indutny
Copy link
Member

indutny commented Oct 10, 2015

@bnoordhuis but it looks like you don't hate to type lots of code to parse hostname out of the single argument?

@bnoordhuis
Copy link
Member Author

No, because I only need to write that once. Think of it as a force multiplier: because I took 15 minutes extra, thousands of people won't have to write --debug-host= every day of their working lives.

@indutny
Copy link
Member

indutny commented Oct 10, 2015

Are there any user bug reports suggesting that this is needed every day of their working lives? It looks to me like this thing will be usually passed by some tooling, except manual by-hand typing.

@bnoordhuis
Copy link
Member Author

Let's not turn this into a bikeshed. Do you have compelling arguments why a new argument is better than extending existing ones? Apart from "it adds code", I mean.

I considered both approaches and decided that I liked this one better. It's less typing and one less CLI argument to remember. I have difficulty enough remembering when to use --debug, --debug-brk and --debug-port.

@indutny
Copy link
Member

indutny commented Oct 10, 2015

Perhaps, some @nodejs/collaborators may help us to resolve this?

@mscdex
Copy link
Contributor

mscdex commented Oct 10, 2015

IMHO I would not expect an option named --debug-port to be checked for a hostname.

On a related note, maybe we could add shorthand/shortened versions of these options for less typing?

@Fishrock123
Copy link
Contributor

I think we should just move to --debug-host. (and a shorthand, maybe -H? I'm not sure what the regular conventions for these things are.)

@bnoordhuis
Copy link
Member Author

Consider this: without the address parsing code, you can write --debug=1234 or --debug-brk=1234 but you'd have to pass an extra argument to set the listen address. Rather incongruous, no?

I'd be okay with either renaming --debug-port or keeping its current behavior. A --debug-host option would still be needed in the second case so I have a a mild preference for renaming.

That said, it's undocumented and internal (it's for the cluster module to configure worker processes) so it doesn't really bother me if the name is a bit off.

@trevnorris
Copy link
Contributor

Personally I'm cool with this syntax. It follows the same syntax as a URL, and doesn't add complexity to visually parse.

@trevnorris
Copy link
Contributor

code changes LGTM

@leitsubomi
Copy link

Has this fix been released to https://deb.nodesource.com/setup_4.x yet? My teammate Yunong filed issue #3306, we want to integrate this fix once it's out. Please keep us informed.

Thanks!

@bnoordhuis
Copy link
Member Author

Resolution from today's Core TC call is that the PR is good to go as is. I'll merge it tomorrow.

@bnoordhuis
Copy link
Member Author

One final CI run because the Windows buildbots were AWOL last time: https://ci.nodejs.org/job/node-test-pull-request/580/

@bnoordhuis
Copy link
Member Author

#2778 introduced a regression that's making this PR fail, see #3585 for a proposed revert.

I had another look at the test from this PR and I realize using default port numbers is unsound when running tests in parallel. I think I'll end up removing the port-less --debug-brk tests but that means no full coverage. Bleh.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@bnoordhuis ... ping

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@bnoordhuis bnoordhuis force-pushed the make-debugger-listen-address-configurable branch from 8df920c to 2e170c6 Compare June 29, 2016 15:07
@bnoordhuis
Copy link
Member Author

Rebased, with a couple of tweaks. The --inspect= switch now also accepts a host name but it doesn't honor it yet. PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/3123/

@bnoordhuis bnoordhuis force-pushed the make-debugger-listen-address-configurable branch from 2e170c6 to 6468809 Compare June 29, 2016 16:43
@MylesBorins
Copy link
Contributor

@bnoordhuis would you be willing to backport to v4.x?

@MylesBorins MylesBorins removed the stalled Issues and PRs that are stalled. label Nov 14, 2016
@MylesBorins MylesBorins removed this from the 4.7.0 milestone Nov 14, 2016
@MylesBorins
Copy link
Contributor

/cc @nodejs/lts @nodejs/collaborators would anyone be comfortable doing a backport of this? Would very much like to get this in before the RC on tuesday

Qard pushed a commit to Qard/node that referenced this pull request Nov 19, 2016
`--debug=1.2.3.4:5678` and `--debug=example.com:5678` are now accepted,
likewise the `--debug-brk` and `--debug-port` switch.  The latter is
now something of a misnomer but it's undocumented and for internal use
only so it shouldn't matter too much.

`--inspect=1.2.3.4:5678` and `--inspect=example.com:5678` are also
accepted but don't use the host name yet; they still bind to the
default address.

Fixes: nodejs#3306
PR-URL: nodejs#3316
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
`--debug=1.2.3.4:5678` and `--debug=example.com:5678` are now accepted,
likewise the `--debug-brk` and `--debug-port` switch.  The latter is
now something of a misnomer but it's undocumented and for internal use
only so it shouldn't matter too much.

`--inspect=1.2.3.4:5678` and `--inspect=example.com:5678` are also
accepted but don't use the host name yet; they still bind to the
default address.

Fixes: #3306
PR-URL: #3316
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
`--debug=1.2.3.4:5678` and `--debug=example.com:5678` are now accepted,
likewise the `--debug-brk` and `--debug-port` switch.  The latter is
now something of a misnomer but it's undocumented and for internal use
only so it shouldn't matter too much.

`--inspect=1.2.3.4:5678` and `--inspect=example.com:5678` are also
accepted but don't use the host name yet; they still bind to the
default address.

Fixes: #3306
PR-URL: #3316
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
This LTS release comes with 108 commits. This includes 30 which are doc
related, 28 which are test related, 16 which are build / tool related,
and 4 commits which are updates to dependencies.

Notable Changes:

The SEMVER-MINOR changes include:

* build:
  - export openssl symbols on Windows making it possible to build
    addons linked against the bundled version of openssl (Alex Hultman)
    #7576
* debugger:
  - make listen address configurable in the debugger server
    (Ben Noordhuis) #3316
* dgram:
  - generalized send queue to handle close fixing a potential throw
    when dgram socket is closed in the listening event handler.
    (Matteo Collina) #7066
* http:
  - Introduce the 451 status code "Unavailable For Legal Reasons"
    (Max Barinov) #4377
* tls:
  - introduce `secureContext` for `tls.connect` which is useful for
    caching client certificates, key, and CA certificates.
    (Fedor Indutny) #4246

Notable SEMVER-PATCH changes include:

* build:
  - introduce the configure --shared option for embedders (sxa555)
    #6994
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* src:
  - node no longer aborts when c-ares initialization fails
    (Ben Noordhuis) #8710
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9736
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
This LTS release comes with 108 commits. This includes 30 which are doc
related, 28 which are test related, 16 which are build / tool related,
and 4 commits which are updates to dependencies.

Notable Changes:

The SEMVER-MINOR changes include:

* build:
  - export openssl symbols on Windows making it possible to build
    addons linked against the bundled version of openssl (Alex Hultman)
    #7576
* debugger:
  - make listen address configurable in the debugger server
    (Ben Noordhuis) #3316
* dgram:
  - generalized send queue to handle close fixing a potential throw
    when dgram socket is closed in the listening event handler.
    (Matteo Collina) #7066
* http:
  - Introduce the 451 status code "Unavailable For Legal Reasons"
    (Max Barinov) #4377
* tls:
  - introduce `secureContext` for `tls.connect` which is useful for
    caching client certificates, key, and CA certificates.
    (Fedor Indutny) #4246

Notable SEMVER-PATCH changes include:

* build:
  - introduce the configure --shared option for embedders (sxa555)
    #6994
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* src:
  - node no longer aborts when c-ares initialization fails
    (Ben Noordhuis) #8710
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9736
MylesBorins pushed a commit that referenced this pull request Dec 6, 2016
This LTS release comes with 108 commits. This includes 30 which are doc
related, 28 which are test related, 16 which are build / tool related,
and 4 commits which are updates to dependencies.

Notable Changes:

The SEMVER-MINOR changes include:

* build:
  - export openssl symbols on Windows making it possible to build
    addons linked against the bundled version of openssl (Alex Hultman)
    #7576
* debugger:
  - make listen address configurable in the debugger server
    (Ben Noordhuis) #3316
* dgram:
  - generalized send queue to handle close fixing a potential throw
    when dgram socket is closed in the listening event handler.
    (Matteo Collina) #7066
* http:
  - Introduce the 451 status code "Unavailable For Legal Reasons"
    (Max Barinov) #4377
* tls:
  - introduce `secureContext` for `tls.connect` which is useful for
    caching client certificates, key, and CA certificates.
    (Fedor Indutny) #4246

Notable SEMVER-PATCH changes include:

* build:
  - introduce the configure --shared option for embedders (sxa555)
    #6994
* gtest:
  - the test reporter now outputs tap comments as yamlish
    (Johan Bergström) #9262
* src:
  - node no longer aborts when c-ares initialization fails
    (Ben Noordhuis) #8710
* tls:
  - fix memory leak when writing data to TLSWrap instance during
    handshake (Fedor Indutny)
    #9586

PR-URL: #9736
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
    This LTS release comes with 108 commits. This includes 30 which are doc
    related, 28 which are test related, 16 which are build / tool related,
    and 4 commits which are updates to dependencies.

    Notable Changes:

    The SEMVER-MINOR changes include:

    * build:
      - export openssl symbols on Windows making it possible to build
        addons linked against the bundled version of openssl (Alex Hultman)
        nodejs/node#7576
    * debugger:
      - make listen address configurable in the debugger server
        (Ben Noordhuis) nodejs/node#3316
    * dgram:
      - generalized send queue to handle close fixing a potential throw
        when dgram socket is closed in the listening event handler.
        (Matteo Collina) nodejs/node#7066
    * http:
      - Introduce the 451 status code "Unavailable For Legal Reasons"
        (Max Barinov) nodejs/node#4377
    * tls:
      - introduce `secureContext` for `tls.connect` which is useful for
        caching client certificates, key, and CA certificates.
        (Fedor Indutny) nodejs/node#4246

    Notable SEMVER-PATCH changes include:

    * build:
      - introduce the configure --shared option for embedders (sxa555)
        nodejs/node#6994
    * gtest:
      - the test reporter now outputs tap comments as yamlish
        (Johan Bergstrom) nodejs/node#9262
    * src:
      - node no longer aborts when c-ares initialization fails
        (Ben Noordhuis) nodejs/node#8710
    * tls:
      - fix memory leak when writing data to TLSWrap instance during
        handshake (Fedor Indutny)
        nodejs/node#9586

    PR-URL: nodejs/node#9736

Signed-off-by: Ilkka Myller <[email protected]>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
    This LTS release comes with 108 commits. This includes 30 which are doc
    related, 28 which are test related, 16 which are build / tool related,
    and 4 commits which are updates to dependencies.

    Notable Changes:

    The SEMVER-MINOR changes include:

    * build:
      - export openssl symbols on Windows making it possible to build
        addons linked against the bundled version of openssl (Alex Hultman)
        nodejs/node#7576
    * debugger:
      - make listen address configurable in the debugger server
        (Ben Noordhuis) nodejs/node#3316
    * dgram:
      - generalized send queue to handle close fixing a potential throw
        when dgram socket is closed in the listening event handler.
        (Matteo Collina) nodejs/node#7066
    * http:
      - Introduce the 451 status code "Unavailable For Legal Reasons"
        (Max Barinov) nodejs/node#4377
    * tls:
      - introduce `secureContext` for `tls.connect` which is useful for
        caching client certificates, key, and CA certificates.
        (Fedor Indutny) nodejs/node#4246

    Notable SEMVER-PATCH changes include:

    * build:
      - introduce the configure --shared option for embedders (sxa555)
        nodejs/node#6994
    * gtest:
      - the test reporter now outputs tap comments as yamlish
        (Johan Bergstrom) nodejs/node#9262
    * src:
      - node no longer aborts when c-ares initialization fails
        (Ben Noordhuis) nodejs/node#8710
    * tls:
      - fix memory leak when writing data to TLSWrap instance during
        handshake (Fedor Indutny)
        nodejs/node#9586

    PR-URL: nodejs/node#9736

Signed-off-by: Ilkka Myller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants