debugger: make listen address configurable #3316
Conversation
|
The change is good, but I'm not sure about the CLI arguments usage. Why not introduce |
|
This will chop off many of the changes in this PR. |
Fishrock123
added
the
semver-minor
Because I hate having to type two arguments when one will do. |
|
@bnoordhuis but it looks like you don't hate to type lots of code to parse hostname out of the single argument? |
|
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 |
|
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. |
|
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 |
|
Perhaps, some @nodejs/collaborators may help us to resolve this? |
|
IMHO I would not expect an option named On a related note, maybe we could add shorthand/shortened versions of these options for less typing? |
|
I think we should just move to |
|
Consider this: without the address parsing code, you can write I'd be okay with either 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. |
|
Personally I'm cool with this syntax. It follows the same syntax as a URL, and doesn't add complexity to visually parse. |
|
code changes LGTM |
|
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! |
|
Resolution from today's Core TC call is that the PR is good to go as is. I'll merge it tomorrow. |
|
One final CI run because the Windows buildbots were AWOL last time: https://ci.nodejs.org/job/node-test-pull-request/580/ |
|
#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 |
|
@bnoordhuis ... ping |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
bnoordhuis
force-pushed
the
make-debugger-listen-address-configurable
branch
from
8df920c
to
2e170c6
Compare
|
Rebased, with a couple of tweaks. The |
bnoordhuis
force-pushed
the
make-debugger-listen-address-configurable
branch
from
2e170c6
to
6468809
Compare
|
@bnoordhuis would you be willing to backport to v4.x? |
|
/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 |
`--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 <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
`--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 <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
`--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 <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
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
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
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
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 <ilkka.myller@nodefield.com>
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 <ilkka.myller@nodefield.com>
Fixes: #3306
R=@indutny
CI: https://ci.nodejs.org/job/node-test-pull-request/476/