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

Calling inspector.open() if the inspector is already available makes inspector.url() give a bad address #33012

Closed
connor4312 opened this issue Apr 22, 2020 · 1 comment

Comments

@connor4312
Copy link
Contributor

  • Version: v12.14.1
  • Platform: win32
  • Subsystem: Inspector

What steps will reproduce the bug?

  1. Create a script foo.js with:

    const inspector = require('inspector');
    console.log(inspector.url());
    inspector.open(0, undefined, false);
    console.log(inspector.url());
  2. Run node foo.js ✔️

    undefined
    Debugger listening on ws://127.0.0.1:37569/25b0bbd1-1b2d-45ab-bcb5-2e509196df2a
    For help, see: https://nodejs.org/en/docs/inspector
    ws://127.0.0.1:37569/25b0bbd1-1b2d-45ab-bcb5-2e509196df2a
    
  3. Run node --inspect foo.js

    Debugger listening on ws://127.0.0.1:9229/f72b422b-dde6-4d57-a507-86f4b7faf919
    For help, see: https://nodejs.org/en/docs/inspector
    ws://127.0.0.1:9229/f72b422b-dde6-4d57-a507-86f4b7faf919
    ws://127.0.0.1:0/f72b422b-dde6-4d57-a507-86f4b7faf919
    

How often does it reproduce? Is there a required condition?

100%

What do you see instead? / What is the expected behavior?

It seems like calling inspect() again will silently "overwrite" the existing run with an unresolved port 0. I would expect any of the following as 'good' behavior:

  • An error is thrown (if we can't open the inspector on the new port)
  • The port is changed to the new address
  • or the inspector.url() is unaffected (no-op)

Additional information

Ref: microsoft/vscode#95128

connor4312 added a commit to microsoft/vscode-js-debug that referenced this issue Apr 22, 2020
See nodejs/node#33012
Fixes microsoft/vscode#95128

We still do remove --inspect-brk since that will break in our
bootloader which entirely blocks attachment.
@joyeecheung
Copy link
Member

joyeecheung commented Apr 23, 2020

I guess in this case, throwing an error warning that the inspector needs to be closed before being opened again should make more sense. However this does break https://github.com/nodejs/node/blame/master/test/sequential/test-inspector-open.js @sam-github is there a particular reason inspector.open() is a no-op (well, not quite, this currently resets the port and host stored in Environment, still nothing meaningful would be done) when the inspector is already active (even though possibly with a different address)?

codebytere pushed a commit that referenced this issue Jun 18, 2020
When the user tries to activate the inspector that is already active
on a different port and host, we previously just silently reset
the port and host stored in the Environment without actually doing
anything for that to be effective. After this patch, we throw
an error telling the user to close the active inspector before invoking
`inspector.open()` again.

PR-URL: #33015
Fixes: #33012
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
When the user tries to activate the inspector that is already active
on a different port and host, we previously just silently reset
the port and host stored in the Environment without actually doing
anything for that to be effective. After this patch, we throw
an error telling the user to close the active inspector before invoking
`inspector.open()` again.

PR-URL: #33015
Fixes: #33012
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this issue Jul 8, 2020
When the user tries to activate the inspector that is already active
on a different port and host, we previously just silently reset
the port and host stored in the Environment without actually doing
anything for that to be effective. After this patch, we throw
an error telling the user to close the active inspector before invoking
`inspector.open()` again.

PR-URL: #33015
Fixes: #33012
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants