Skip to content

Conversation

@hybrist
Copy link
Contributor

@hybristhybrist 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

  • 42e0cd1fix: look for generic hint text

1.11.1

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

@hybrist
Copy link
ContributorAuthor

hybrist 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) 

@hybristhybrist mentioned this pull request Apr 12, 2017
14 tasks
Copy link
Contributor

@aqrlnaqrln 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

@TrottTrott 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

@hybrist
Copy link
ContributorAuthor

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

@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

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.

@hybrist
Copy link
ContributorAuthor

@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

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

@hybristhybrist changed the title deps: update node-inspect to v1.11.1deps: update node-inspect to v1.11.2Apr 14, 2017
@hybrist
Copy link
ContributorAuthor

@joshgav Done!

@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in 0217197

addaleax pushed a commit that referenced this pull request Apr 14, 2017
PR-URL: #12363 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]>
@hybristhybrist deleted the jk-node-inspect-1.11.1 branch April 15, 2017 00:48
@joshgavjoshgav mentioned this pull request Apr 19, 2017
4 tasks
@jasnelljasnell mentioned this pull request May 11, 2017
@gibfahngibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

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 freeto 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

@hybrist@aqrln@Trott@mscdex@joshgav@addaleax@gibfahn@jasnell@cjihrig