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
base: master
from

Conversation

Projects
None yet
9 participants
@jkrems
Contributor

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

@mscdex mscdex added the debugger label Apr 12, 2017

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 12, 2017

Contributor

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

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)
    

@jkrems jkrems referenced this pull request Apr 12, 2017

Closed

Switch the CLI debugger to V8 inspector #11421

14 of 14 tasks complete
@aqrln

aqrln approved these changes Apr 12, 2017

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 😄

@Trott

Trott approved these changes Apr 12, 2017

Rubber-stamp LGTM if all tests pass in CI

@aqrln

This comment has been minimized.

Show comment
Hide comment
@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 12, 2017

Contributor

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)
  ...
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 12, 2017

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/

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 Trott referenced this pull request Apr 12, 2017

Closed

Remove `common.PORT` usage from `parallel` tests #12376

55 of 64 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 12, 2017

Member

New CI is green, woot.

Member

Trott commented Apr 12, 2017

New CI is green, woot.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 12, 2017

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 12, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Apr 14, 2017

Member

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

Member

joshgav commented Apr 14, 2017

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

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

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 14, 2017

Contributor

@joshgav Done!

Contributor

jkrems commented Apr 14, 2017

@joshgav Done!

@addaleax

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented Apr 14, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 14, 2017

Member

Landed in 0217197

Member

addaleax commented Apr 14, 2017

Landed in 0217197

@addaleax addaleax closed this Apr 14, 2017

addaleax added a commit that referenced this pull request Apr 14, 2017

deps: update node-inspect to v1.11.2
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 jkrems:jk-node-inspect-1.11.1 branch Apr 15, 2017

@joshgav joshgav referenced this pull request Apr 19, 2017

Closed

src: update --inspect hint text #11207

4 of 4 tasks complete

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 18, 2017

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.

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