debugger: make listen address configurable#3316
Uh oh!
There was an error while loading. Please reload this page.
Conversation
indutny commented Oct 10, 2015
The change is good, but I'm not sure about the CLI arguments usage. Why not introduce |
indutny commented Oct 10, 2015
This will chop off many of the changes in this PR. |
Fishrock123 added the semver-minor bnoordhuis commented Oct 10, 2015
Because I hate having to type two arguments when one will do. |
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 commented Oct 10, 2015
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 |
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 commented Oct 10, 2015
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 |
indutny commented Oct 10, 2015
Perhaps, some @nodejs/collaborators may help us to resolve this? |
mscdex commented Oct 10, 2015
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? |
Fishrock123 commented Oct 10, 2015
I think we should just move to |
bnoordhuis commented Oct 11, 2015
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. |
trevnorris commented Oct 12, 2015
Personally I'm cool with this syntax. It follows the same syntax as a URL, and doesn't add complexity to visually parse. |
trevnorris commented Oct 12, 2015
code changes LGTM |
leitsubomi commented Oct 21, 2015
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 commented Oct 21, 2015
Resolution from today's Core TC call is that the PR is good to go as is. I'll merge it tomorrow. |
bnoordhuis commented Oct 23, 2015
One final CI run because the Windows buildbots were AWOL last time: https://ci.nodejs.org/job/node-test-pull-request/580/ |
bnoordhuis commented Oct 29, 2015
#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 |
jasnell commented Mar 22, 2016
@bnoordhuis ... ping |
estliberitasforce-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompare
bnoordhuisforce-pushed the make-debugger-listen-address-configurable branch from 8df920c to 2e170c6Comparebnoordhuis commented Jun 29, 2016
Rebased, with a couple of tweaks. The |
bnoordhuisforce-pushed the make-debugger-listen-address-configurable branch from 2e170c6 to 6468809Compare
MylesBorins commented Nov 14, 2016
@bnoordhuis would you be willing to backport to v4.x? |
MylesBorins commented Nov 18, 2016
/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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
`--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]>
`--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]>
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 <[email protected]>
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]>
8 participants
Fixes: #3306
R=@indutny
CI: https://ci.nodejs.org/job/node-test-pull-request/476/