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

Allow to suppress --inspect hint #12665

Closed
segrey opened this issue Apr 26, 2017 · 22 comments
Closed

Allow to suppress --inspect hint #12665

segrey opened this issue Apr 26, 2017 · 22 comments
Labels
feature request Issues that request new features to be added to Node.js. inspector Issues and PRs related to the V8 inspector protocol stalled Issues and PRs that are stalled.

Comments

@segrey
Copy link

segrey commented Apr 26, 2017

When started with inspector protocol, node emits messages relevant for debugging in Chrome DevTools:

node --inspect=48235 --debug-brk test.js
Debugger listening on port 48235.
Warning: This is an experimental feature and could change at any time.
To start debugging, open the following URL in Chrome:
    chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:48235/c80523d4-0bd2-4e43-8af0-8dc75f12eeb3

However, these messages are not relevant when debugging using a tool (e.g. IDE) that connects to the debug port immediately.
It would be nice if it would be possible to suppress the hint. Possible ways:

  1. Check some environment variable (not great, but possible)
  2. Or print the hint text only if no connections within 1-2 seconds (that should be enough time for a tool to connect).
  • Version: 7.9.0
  • Platform: Linux segrey-desktop 3.19.0-32-generic 37~14.04.1-Ubuntu SMP Thu Oct 22 09:41:40 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: debugger
@segrey
Copy link
Author

segrey commented Apr 26, 2017

Currently, tools can analyze stderr stream and chop off the messages. Since messages format is not strictly defined, it looks like a guessing on tools' side. E.g. hint text format is going to be changed in #11207.

@gibfahn gibfahn added the inspector Issues and PRs related to the V8 inspector protocol label Apr 26, 2017
@Fishrock123
Copy link
Member

Fishrock123 commented Apr 26, 2017

I think you can --debug-brk and then send a SIGUSR1 signal?

@cjihrig
Copy link
Contributor

cjihrig commented Apr 26, 2017

On current master, it looks like --debug and --debug-brk no longer exist. I think there is some ongoing discussion about those flags, but right now I'm getting "bad option"

@segrey
Copy link
Author

segrey commented Apr 26, 2017

@cjihrig Correct, on current master no --debug-brk. The issue can be reproduced without --debug-brk as well.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 26, 2017

Right. @Fishrock123 was proposing an existing solution to the problem.

@segrey
Copy link
Author

segrey commented Apr 26, 2017

@cjihrig Thank you for looking into it and the PR! What do you think about postponing printing the hint text for some time to let tools connect the the port? Seems there is already a logic that prints the hint when a client disconnects.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 26, 2017

If you mean adding an arbitrary timeout before printing, I'm -1 on that idea. It's difficult to add testing for, and can be affected by the machine (as we've seen in our own CI).

@segrey
Copy link
Author

segrey commented Apr 26, 2017

Might be helpful for VSCode /cc @roblourens

@roblourens
Copy link

Yeah, currently VS Code snips out the message from the output stream. I probably wouldn't use --inspect-silent at least for awhile, because of edge cases in determining whether we are launching a version of node that supports it (same issues from #12364).

@segrey
Copy link
Author

segrey commented Apr 27, 2017

Right, makes sense.

@segrey
Copy link
Author

segrey commented Apr 27, 2017

@Fishrock123 IIUC, passing --debug-brk (and signaling a process with SIGUSR1) will enable old protocol. Here inspector protocol is targeted.

@Trott
Copy link
Member

Trott commented Aug 4, 2017

This is still an issue, right? No canonical way to squelch the output? Or should this be closed?

@cjihrig
Copy link
Contributor

cjihrig commented Aug 4, 2017

I closed #12671 without merging because we couldn't really decide how this should be implemented. I'm happy to reopen it if we come up with a plan first.

@targos targos added feature request Issues that request new features to be added to Node.js. stalled Issues and PRs that are stalled. labels Nov 4, 2018
@Trott Trott added this to backlog/orphan in feature requests via automation Nov 25, 2018
@Trott
Copy link
Member

Trott commented Nov 25, 2018

Added to the Feature Requests project. Closing as stalled/no-consensus.

@Trott Trott closed this as completed Nov 25, 2018
feature requests automation moved this from backlog/orphan to Done Nov 25, 2018
@refack refack moved this from Done to backlog/orphan in feature requests May 23, 2019
@connor4312
Copy link
Contributor

I'm interested in revisiting this for VS Code. Our new debugger uses NODE_OPTIONS to --require a bootloader that hooks up processes to our debugger, which lets us have a 'magic' console where everything executed in it, including child processes of top-level processes, are debugged. Here, the debug adapter doesn't intercept the stdio, so the approach that Rob mentioned where we snip the message out doesn't work.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 1, 2020

FWIW, I'd still like to revisit #12671 to address this.

@Trott Trott reopened this Feb 1, 2020
feature requests automation moved this from backlog/orphan to In progress Feb 1, 2020
@connor4312
Copy link
Contributor

Wanted to follow up on this and see if there's anything I can do to help 🙂 We're interested in making all terminals 'debug' terminals, but if we're unable to suppress the inspector messages this risks clogging up the output with lots of irrelevant (to the user) messages.

An alternative approach sufficient for VS Code--if it would be more palatable to the Node.js team--is to add a function in the inspector module or an argument to inspector.open that would suppress the message when invoked. However, the environment variable approach is more generic.

@jasnell
Copy link
Member

jasnell commented Jul 6, 2020

I don't believe this issue is relevant any longer. The current message printed is agnostic of the debugger used:

C:\Users\jasne\Projects\node>node --inspect=48235
Debugger listening on ws://127.0.0.1:48235/054c83e9-cd6c-4db7-8b66-3d224edada25
For help, see: https://nodejs.org/en/docs/inspector
Welcome to Node.js v14.5.0.
Type ".help" for more information.

Closing this.

@jasnell jasnell closed this as completed Jul 6, 2020
@connor4312
Copy link
Contributor

connor4312 commented Jul 6, 2020

@jasnell that's the problem -- we'd like the ability to not show that message.

@bnoordhuis
Copy link
Member

@connor4312 You can now: node --inspect --inspect-publish-uid=http script.js. See #27741.

@connor4312
Copy link
Contributor

connor4312 commented Aug 18, 2020

@bnoordhuis it looks like that's invalid in NODE_OPTIONS, so it doesn't work for our use case in VS Code where we attach the debugger using the options:

PS C:\Users\Connor\Documents\Github\vscode-js-debug\demos\node> node
C:\Program Files\nodejs\node.exe: --inspect-public-uid= is not allowed in NODE_OPTIONS  

While we could add this manually to the top level program being executed, one big sore spot for which we're interested in this is child process, which we debug by --require'ing a bootloader in NODE_OPTIONS. Currently running a process that spawns a bunch of children looks something like this:

Should we reopen this issue or should I file a new issue to request the --inspect-public-uid be allowed in NODE_OPTIONS?

connor4312 added a commit to microsoft/vscode-js-debug that referenced this issue Aug 18, 2020
@bnoordhuis
Copy link
Member

bnoordhuis commented Aug 19, 2020

@connor4312 I'd open a pull request whitelisting it in src/node_options.cc. Node already allows --inspect and --inspect-port. It makes sense to also allow --inspect-public-uid.

edit: I mean this list:

node/src/node_options.cc

Lines 248 to 258 in 6e97a73

AddOption("--inspect-port",
"set host:port for inspector",
&DebugOptions::host_port,
kAllowedInEnvironment);
AddAlias("--debug-port", "--inspect-port");
AddOption("--inspect",
"activate inspector on host:port (default: 127.0.0.1:9229)",
&DebugOptions::inspector_enabled,
kAllowedInEnvironment);
AddAlias("--inspect=", { "--inspect-port", "--inspect" });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. inspector Issues and PRs related to the V8 inspector protocol stalled Issues and PRs that are stalled.
Projects
No open projects
feature requests
  
In progress
Development

No branches or pull requests

10 participants