debugger: propagate --debug-port= to debuggee #3470

bnoordhuis · 2015-10-21T12:00:32Z

Before this commit node --debug-port=1234 debug t.js ignored the
--debug-port= argument, binding to the default port 5858 instead,
making it impossible to debug more than one process on the same
machine that way.

This commit also reduces the number of places where the default port
is hard-coded by one.

Fixes: #3345

R=@indutny

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

indutny · 2015-10-21T13:51:43Z

LGTM

bnoordhuis · 2015-10-21T16:29:41Z

One more CI with a fix-up for Windows path munging in the test: https://ci.nodejs.org/job/node-test-pull-request/551/

jasnell · 2015-10-21T16:59:06Z

LGTM

bnoordhuis · 2015-10-21T17:31:29Z

It looks like the test is hitting a race condition in the debugger where the child processes get stuck waiting on a semaphore...

(gdb) thread apply 1 bt 10

Thread 1 (Thread 0x7f92e89e6740 (LWP 17569)):
#0  0x00007f92e792a829 in do_futex_wait.constprop () from /lib64/libpthread.so.0
#1  0x00007f92e792a8c4 in __new_sem_wait_slow.constprop.0 () from /lib64/libpthread.so.0
#2  0x00007f92e792a96a in sem_wait@@GLIBC_2.2.5 () from /lib64/libpthread.so.0
#3  0x0000000000e11728 in v8::base::Semaphore::Wait() ()
#4  0x00000000009b3ad4 in v8::internal::Debug::NotifyMessageHandler(v8::DebugEvent, v8::internal::Handle<v8::internal::JSObject>, v8::internal::Handle<v8::internal::JSObject>, bool) ()
#5  0x00000000009b44c8 in v8::internal::Debug::ProcessDebugEvent(v8::DebugEvent, v8::internal::Handle<v8::internal::JSObject>, bool) ()
#6  0x00000000009b45c9 in v8::internal::Debug::OnDebugBreak(v8::internal::Handle<v8::internal::Object>, bool) ()
#7  0x00000000009b616b in v8::internal::Debug::Break(v8::internal::Arguments, v8::internal::JavaScriptFrame*) ()
#8  0x0000000000c01be3 in v8::internal::Runtime_DebugBreak(int, v8::internal::Object**, v8::internal::Isolate*) ()
#9  0x000010de5e0060bb in ?? ()
(More stack frames follow...)
(gdb) thread apply 2 bt 10

Thread 2 (Thread 0x7f92e5557700 (LWP 17589)):
#0  0x00007f92e7658eb9 in syscall () from /lib64/libc.so.6
#1  0x0000000000e0d18a in uv__epoll_wait (epfd=<optimized out>, events=events@entry=0x7f92e5553d50, nevents=nevents@entry=1024, timeout=timeout@entry=-1)
    at ../deps/uv/src/unix/linux-syscalls.c:321
#2  0x0000000000e0b04e in uv__io_poll (loop=loop@entry=0x26c85e0, timeout=-1) at ../deps/uv/src/unix/linux-core.c:243
#3  0x0000000000dfc5a2 in uv_run (loop=0x26c85e0, mode=UV_RUN_DEFAULT) at ../deps/uv/src/unix/core.c:341
#4  0x0000000000d648c1 in node::debugger::Agent::WorkerRun() ()
#5  0x0000000000e07ac8 in uv__thread_start (arg=<optimized out>) at ../deps/uv/src/unix/thread.c:49
#6  0x00007f92e7923555 in start_thread () from /lib64/libpthread.so.0
#7  0x00007f92e765eb9d in clone () from /lib64/libc.so.6

Happens locally about once every ~50 runs. Will investigate.

EDIT: Interestingly enough I've also seen cases where the main thread is already gone but the debugger thread is still around, sleeping in epoll_wait.

EDIT2: It may be much more trivial than that. Looks like killing the debugger doesn't always terminate the debuggee.

MylesBorins · 2016-03-10T19:46:29Z

@bnoordhuis what ended up happening with this?

bnoordhuis · 2016-03-23T17:28:26Z

@thealphanerd It's basically blocked by #5368 - v8::Debug::DebugBreak() is no longer async signal-safe. I'm going to revisit this PR once that issue is fixed - if it gets fixed.

Trott · 2016-05-24T22:37:23Z

@bnoordhuis #5368 was closed by #5904. Does that mean this is ready to move forward again? Or is life not that simple?

bnoordhuis · 2016-05-25T08:15:12Z

This should be good to go again. I actually had "dusting off debugger PRs" on my TODO list for this week.

Rebased + new CI: https://ci.nodejs.org/job/node-test-pull-request/2778/

cjihrig · 2016-05-25T15:55:40Z

LGTM. There were some CI failures that all look unrelated, but the number of failures might warrant another CI.

Trott · 2016-05-25T17:38:47Z

The (hopefully unrelated) failures are all EADDRINUSE on localhost:common.PORT. Since this adds a test that uses that address/port, it's Not Impossible that it doesn't let that port go from time to time and causes the other tests to fail. I kind of doubt it, but since debugger tests are pretty much the most unpredictable thing that has ever existed in terms of side effects, let's try again: https://ci.nodejs.org/job/node-test-pull-request/2786/

bnoordhuis · 2016-05-25T19:43:32Z

Le sigh, Java exceptions on the ARM bots. Third time is the charm: https://ci.nodejs.org/job/node-test-pull-request/2788/

Trott · 2016-05-25T20:26:41Z

Probably fine to table-flip the CI and land the code at this point but hey, I loves me some CI runs, so here's another one: https://ci.nodejs.org/job/node-test-pull-request/2790/

Before this commit `node --debug-port=1234 debug t.js` ignored the --debug-port= argument, binding to the default port 5858 instead, making it impossible to debug more than one process on the same machine that way. This commit also reduces the number of places where the default port is hard-coded by one. Fixes: nodejs#3345 PR-URL: nodejs#3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis · 2016-05-25T21:41:36Z

This time it was the ARM bots and one of the plinux machines... =)

I'm confident enough it works though. Landed in 18fb4f9 and added the lts-watch-v4.x tag. Thanks for the reviews, everyone.

Trott · 2016-05-28T04:42:00Z

The (hopefully unrelated) failures are all EADDRINUSE on localhost:common.PORT. Since this adds a test that uses that address/port, it's Not Impossible that it doesn't let that port go from time to time and causes the other tests to fail.

It looks like that may be exactly what's going on and why (or at least one reason why) the last couple days have been unpleasant in CI-land...

https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd102-64/748/console

+ OK=5
+ echo '5   OK: 5   NOT OK: 0   TOTAL: 999'
5   OK: 5   NOT OK: 0   TOTAL: 999
+ for i in '`seq $RUN_TIMES`'
+ python tools/test.py -p tap --mode=release parallel/test-debug-port-numbers
1..1
ok 1 parallel/test-debug-port-numbers
  ---
  duration_ms: 1.376
  ...
+ OK=6
+ echo '6   OK: 6   NOT OK: 0   TOTAL: 999'
6   OK: 6   NOT OK: 0   TOTAL: 999
+ for i in '`seq $RUN_TIMES`'
+ python tools/test.py -p tap --mode=release parallel/test-debug-port-numbers
1..1
not ok 1 parallel/test-debug-port-numbers
# TIMEOUT
# debug> debug> debug> debug> �< Debugger listening on port 12346�< Debugger listening on port 12347
# debug> �connecting to 127.0.0.1:12347 ...
# ��debug> < Debugger listening on port 12348< Error: listen EADDRINUSE :::12349�connecting to 127.0.0.1:12346 ...
# debug> �connecting to 127.0.0.1:12348 ..
# debug> �connecting to 127.0.0.1:12349 .... ok
# debug>  ok
# debug>  ok
# debug> �<     at Object.exports._errnoException (util.js:1007:11)
# <     at exports._exceptionWithHostPort (util.js:1030:20)
# <     at Agent.Server._listen2 (net.js:1253:14)
# <     at listen (net.js:1289:10)
# <     at Agent.Server.listen (net.js:1385:5)
# <     at Object.start (_debug_agent.js:21:9)
# <     at startup (node.js:74:44)
# <     at node.js:444:3
# debug>  ok
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> 
  ---
  duration_ms: 60.65
  ...
+ NOK=1
+ echo '7   OK: 6   NOT OK: 1   TOTAL: 999'
7   OK: 6   NOT OK: 1   TOTAL: 999
+ for i in '`seq $RUN_TIMES`'
+ python tools/test.py -p tap --mode=release parallel/test-debug-port-numbers
1..1
not ok 1 parallel/test-debug-port-numbers
# TIMEOUT
# debug> debug> debug> debug> ��< Error: listen EADDRINUSE :::12349
# < Debugger listening on port 12347debug> �connecting to 127.0.0.1:12349 ..
# .�< Debugger listening on port 12346
# debug> �connecting to 127.0.0.1:12347 ...��< Debugger listening on port 12348<     at Object.exports._errnoException (util.js:1007:11)
# <     at exports._exceptionWithHostPort (util.js:1030:20)
# <     at Agent.Server._listen2 (net.js:1253:14)
# <     at listen (net.js:1289:10)
# <     at Agent.Server.listen (net.js:1385:5)
# <     at Object.start (_debug_agent.js:21:9)
# <     at startup (node.js:74:44)
# <     at node.js:444:3
# 
# debug> debug> �connecting to 127.0.0.1:12348 ..debug>  ok
# . ok
# debug> debug> �connecting to 127.0.0.1:12346 ... ok
# debug>  ok
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug> �break in test/parallel/test-debug-port-numbers.js:1
# �> 1 'use strict';
# �  2 
# �  3 const common = require('../common');
# debug>

bnoordhuis · 2016-05-28T09:35:31Z

Continues in #7034.

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: nodejs#7034 Refs: nodejs#3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: #7034 PR-URL: #7037 Refs: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Before this commit `node --debug-port=1234 debug t.js` ignored the --debug-port= argument, binding to the default port 5858 instead, making it impossible to debug more than one process on the same machine that way. This commit also reduces the number of places where the default port is hard-coded by one. Fixes: nodejs#3345 PR-URL: nodejs#3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: nodejs#7034 PR-URL: nodejs#7037 Refs: nodejs#3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Before this commit `node --debug-port=1234 debug t.js` ignored the --debug-port= argument, binding to the default port 5858 instead, making it impossible to debug more than one process on the same machine that way. This commit also reduces the number of places where the default port is hard-coded by one. Fixes: #3345 PR-URL: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: #7034 PR-URL: #7037 Refs: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Before this commit `node --debug-port=1234 debug t.js` ignored the --debug-port= argument, binding to the default port 5858 instead, making it impossible to debug more than one process on the same machine that way. This commit also reduces the number of places where the default port is hard-coded by one. Fixes: #3345 PR-URL: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: #7034 PR-URL: #7037 Refs: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Before this commit `node --debug-port=1234 debug t.js` ignored the --debug-port= argument, binding to the default port 5858 instead, making it impossible to debug more than one process on the same machine that way. This commit also reduces the number of places where the default port is hard-coded by one. Fixes: #3345 PR-URL: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

On UNIX platforms, the debugger doesn't reliably kill the inferior when killed by a signal. Work around that by spawning the debugger in its own process group and killing the process group instead of just the debugger process. This is a hack to get the continuous integration back to green, it doesn't address the underlying issue, which is that the debugger shouldn't leave stray processes behind. Fixes: #7034 PR-URL: #7037 Refs: #3470 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

bnoordhuis added the debugger label Oct 21, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22

bnoordhuis force-pushed the fix3345 branch from 88355b5 to bdf5fbc Compare May 25, 2016 08:13

bnoordhuis force-pushed the fix3345 branch from bdf5fbc to 18fb4f9 Compare May 25, 2016 21:40

bnoordhuis closed this May 25, 2016

bnoordhuis deleted the fix3345 branch May 25, 2016 21:40

bnoordhuis merged commit 18fb4f9 into nodejs:master May 25, 2016

bnoordhuis added the lts-watch-v4.x label May 25, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jun 29, 2016

MylesBorins mentioned this pull request Jul 12, 2016

v4.5.0 proposal #7688

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debugger: propagate --debug-port= to debuggee #3470

debugger: propagate --debug-port= to debuggee #3470

bnoordhuis commented Oct 21, 2015

indutny commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

jasnell commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

MylesBorins commented Mar 10, 2016

bnoordhuis commented Mar 23, 2016

Trott commented May 24, 2016

bnoordhuis commented May 25, 2016

cjihrig commented May 25, 2016

Trott commented May 25, 2016

bnoordhuis commented May 25, 2016

Trott commented May 25, 2016

bnoordhuis commented May 25, 2016

Trott commented May 28, 2016

bnoordhuis commented May 28, 2016

debugger: propagate --debug-port= to debuggee #3470

debugger: propagate --debug-port= to debuggee #3470

Conversation

bnoordhuis commented Oct 21, 2015

indutny commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

jasnell commented Oct 21, 2015

bnoordhuis commented Oct 21, 2015

MylesBorins commented Mar 10, 2016

bnoordhuis commented Mar 23, 2016

Trott commented May 24, 2016

bnoordhuis commented May 25, 2016

cjihrig commented May 25, 2016

Trott commented May 25, 2016

bnoordhuis commented May 25, 2016

Trott commented May 25, 2016

bnoordhuis commented May 25, 2016

Trott commented May 28, 2016

bnoordhuis commented May 28, 2016