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

deps: update node-inspect to v1.11.2 #12363

Closed
wants to merge 2 commits into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Apr 12, 2017

This updates the bundled node-inspect to 1.11.2, including nodejs/node-inspect#43 which was previously cherry-picked in #11441.

Highlights

  • Support for debugging a pid (node inspect -p <pid>)
  • Fixes to support latest master changes
  • Memory & CPU profiling added to help text

Compare: nodejs/node-inspect@v1.10.6...v1.11.2

Rendered Changelog

1.11.2

  • 42e0cd1 fix: look for generic hint text

1.11.1

  • Prefer --inspect-brk over --debug-brk - @ofrobots #43
    • 2c1ed27 fix: use --inspect-brk with Node 8+

1.11.0

Checklist
  • make -j4 test and make test-node-inspect pass
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

@jkrems
Copy link
Contributor Author

jkrems commented Apr 12, 2017

Example for debugging a pid:

  1. Start the debug target and retrieve its pid:

    $ ./node -e 'setInterval(() => { debugger; console.log("ok"); }, 5000); console.log(process.pid)'
    <pid>
    ok
    
  2. Attach the CLI debugger to the process in a separate shell session:

    $ ./node inspect -p <pid>
    break in [eval]:1
    > 1 setInterval(() => { debugger; console.log("ok"); }, 5000); console.log(process.pid)
    

Copy link
Contributor

@aqrln aqrln left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Oh, and this PR reminded me that I was interested in writing some tests and improving autocompletion of object properties, so thank you for this incidental reminder too 😄

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rubber-stamp LGTM if all tests pass in CI

@aqrln
Copy link
Contributor

aqrln commented Apr 12, 2017

@jkrems
Copy link
Contributor Author

jkrems commented Apr 12, 2017

freebsd failure:

not ok 728 parallel/test-net-connect-local-error
  ---
  duration_ms: 0.193
  severity: fail
  stack: |-
    
    assert.js:82
      throw new assert.AssertionError({
      ^
    AssertionError: undefined !== 12346 in Error: connect EADDRINUSE 127.0.0.1:12347 - Local (127.0.0.1:12346)
        at Socket.onError (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-connect-local-error.js:13:10)
        at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common.js:461:15)
        at emitOne (events.js:115:13)
        at Socket.emit (events.js:210:7)
        at emitErrorNT (net.js:1305:8)
        at _combinedTickCallback (internal/process/next_tick.js:80:11)
        at process._tickCallback (internal/process/next_tick.js:104:9)
  ...

@Trott
Copy link
Member

Trott commented Apr 12, 2017

@jkrems My current guess as to what happened here: parallel/test-net-connect-options-allowhalfopen started running slightly before parallel/test-net-connect-local-error (but finished slightly after as it took almost 200ms longer to run) and it's server.listen(0, ...) happened to get assigned port 12346, conflicting with the use of common.PORT in parallel/test-net-connect-local-error.

So, in brief, unrelated to this change (which shouldn't be a surprise, I suppose).

I'll re-run CI and I'll also see about perhaps moving all use of common.PORT into sequential or else adjusting the tests to use port 0. (I think someone--@mscdex or @cjihrig, maybe?--already went through and made as many tests as could be trivially made to use port 0 do so, but I'll look anyway, just in case I'm misremembering.)

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

@Trott
Copy link
Member

Trott commented Apr 12, 2017

New CI is green, woot.

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2017

@Trott Yep, AFAIK the remaining tests still using common.PORT are cluster-related tests because of an outstanding issue with assigning random ports for cluster workers.

@jkrems
Copy link
Contributor Author

jkrems commented Apr 12, 2017

@Trott Thanks for looking into this! I assumed it was something along those lines but didn't have time yet to take a look myself.

@joshgav
Copy link
Contributor

joshgav commented Apr 14, 2017

@jkrems can we pull in nodejs/node-inspect#44 too? Thanks!

@jkrems jkrems changed the title deps: update node-inspect to v1.11.1 deps: update node-inspect to v1.11.2 Apr 14, 2017
@jkrems
Copy link
Contributor Author

jkrems commented Apr 14, 2017

@joshgav Done!

@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in 0217197

@addaleax addaleax closed this Apr 14, 2017
addaleax pushed a commit that referenced this pull request Apr 14, 2017
PR-URL: #12363
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@jkrems jkrems deleted the jk-node-inspect-1.11.1 branch April 15, 2017 00:48
@joshgav joshgav mentioned this pull request Apr 19, 2017
4 tasks
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

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 this pull request may close these issues.

9 participants