debugger: fix stuck debugger when debuggee exits #6332

reshnm · 2016-04-21T15:57:17Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

debugger

Description of change

Referencing PR #27778

The debuggee node process will never exit because the execution
is blocked inside node::debugger::Agent::Stop,
joining the agent thread.
The debug agent thread is blocked inside uv_run.

To fix this, the debug agent has to close all client connections
in _debug_agent.js (process._debugAPI.onclose).

Additionally all remaining opened handles have to be closed
before calling uv_loop_close.
Otherwise uv_loop_close will fail with UV_EBUSY.

Assume the following script (debug.js):

console.log('debug');

Calling node debug debug.js produces the following output:

< Debugger listening on port 5858
connecting to 127.0.0.1:5858 ... ok
break in debug.js:1
> 1 console.log('debug');
  2 
  3 });
debug> c
< debug
program terminated
debug>

Both

node --debug -e 0

and

node --debug-brk -e 0

exit immediately.

The debuggee node process will never exit because the execution is blocked inside node::debugger::Agent::Stop, joining the agent thread. The debug agent thread is blocked inside uv_run. To fix this, the debug agent has to close all client connections in _debug_agent.js (process._debugAPI.onclose). Additionally all remaining opened handles have to be closed before calling uv_loop_close. Otherwise uv_loop_close will fail with UV_EBUSY.

MylesBorins · 2016-04-21T16:35:34Z

lib/_debug_agent.js

@@ -81,6 +85,12 @@ Agent.prototype.notifyWait = function notifyWait() {
  this.first = false;
 };

+Agent.prototype.destroyAllClients = function destroyAllClients() {
+  this.clients.forEach(function(client) {
+    client.destroy();


this ends up calling this.socket.destroy

I didn't dig too much further, but is this operation synchronous? If not there might be some unexpected behavior here

/cc @bnoordhuis

I will dig into this. I used destroy, because it was already used in the close event.
Maybe this.socket.end() would be sufficient here.

Edit: It seems that this.socket.end() also works. But it eventually will also call destroy.

MylesBorins · 2016-04-21T16:36:00Z

@quaidn amazing!!! this is a problem I've wanted to chase down for a bit but never quite got around to it. How comfortable would you feel making a test for this?

Here is a test I wrote for a regression in util that might be useful as a starting point
https://github.com/nodejs/node/blob/master/test/parallel/test-debugger-util-regression.js

If we can get a test that doesn't work on master, but does work with this change we should be able to get this landed!

MylesBorins · 2016-04-21T16:37:32Z

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

reshnm · 2016-04-21T17:03:44Z

@thealphanerd I agree. A test would be good here. I try to use your script as a starting point.

MylesBorins · 2016-04-21T17:04:31Z

@quaidn amazing. Please feel free to let me know if there is anything I can do to help

Added a regression test for stuck debugger when the debuggee exits. The test does several cycles of 'continue' and 'run'. The test passes if the debuggee was terminated within the timeout.

reshnm · 2016-04-22T07:32:27Z

@thealphanerd I added a regression test. Let me know what you think.

On master without these changes, the timeout is hit.

reshnm · 2016-04-22T14:24:03Z

src/debug-agent.cc

@@ -149,6 +154,9 @@ void Agent::Stop() {
  CHECK_EQ(err, 0);

  uv_close(reinterpret_cast<uv_handle_t*>(&child_signal_), nullptr);
+  // Close all remaining handles:
+  // uv_loop_close will return UV_EBUSY for handles which are not closed.
+  uv_walk(&child_loop_, close_handle, nullptr);


This is a workaround. I can't explain why there are always two pipe handles left referenced.

[--I] signal 0x30606e8 [-AI] async 0x3060530 [---] async 0x30603e8 [R--] pipe 0x7fd8a0058520 [R--] pipe 0x7fd8a0064260

They are the stdout and stderr pipes. Closing them like that is not very optimal, it leaves their PipeWrap instances in an invalid state.

Proper cleanup is one of the things I have to tackle for the multi-isolate work. I'll try to get around to it later this week.

MylesBorins · 2016-05-09T21:41:44Z

@bnoordhuis is this something you have time to take a look at?

MylesBorins · 2016-06-06T22:09:54Z

@Trott any thoughts on this?

Trott · 2016-06-06T23:58:31Z

test/parallel/test-debugger-stuck-regression.js

+  fixture
+];
+
+const TEST_TIMEOUT_MS = 4000;


Nit: Maybe use common.platformTimeout(4000) so Raspberry Pi devices get a little extra time in CI.

Trott · 2016-06-07T00:02:32Z

Pinging @indutny on the C++ changes.

jjqq2013 · 2016-06-28T05:11:42Z

+1

Use common.platformTimout to give ARM devices extra time for test execution.

MylesBorins · 2016-06-28T08:31:30Z

/cc @nodejs/diagnostics

MylesBorins · 2016-07-05T17:43:37Z

ping @nodejs/diagnostics @bnoordhuis

MylesBorins · 2016-12-29T18:05:15Z

ping @nodejs/diagnostics @bnoordhuis

might make sense to fix this in LTS

jasnell · 2017-03-01T01:22:42Z

Does this need to stay open?

jasnell · 2017-03-24T22:15:55Z

Marking this stalled. Will close soon if there is no further activity

bnoordhuis · 2017-03-26T11:07:52Z

The multi-isolate work didn't go anywhere but the issue remains that blindly closing all libuv handles is a bad idea (as in 'segfault bad' and 'silent data corruption bad'.)

My suggestion would be to do nothing and sit it out. The old debugger is going away and the inspector doesn't have this issue.

bnoordhuis · 2017-08-17T14:00:20Z

The old debugger has been removed. I'll close this out. Thanks for the PR though.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 21, 2016

mscdex added debugger and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Apr 21, 2016

MylesBorins reviewed Apr 21, 2016
View reviewed changes

MylesBorins added the confirmed-bug Issues with confirmed bugs. label Apr 21, 2016

debugger: added test for stuck debugger

56d54d3

Added a regression test for stuck debugger when the debuggee exits. The test does several cycles of 'continue' and 'run'. The test passes if the debuggee was terminated within the timeout.

reshnm reviewed Apr 22, 2016
View reviewed changes

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

MylesBorins self-assigned this May 9, 2016

Trott reviewed Jun 6, 2016
View reviewed changes

debugger: use common.platformTimeout

d9d6294

Use common.platformTimout to give ARM devices extra time for test execution.

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

bnoordhuis closed this Aug 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

debugger: fix stuck debugger when debuggee exits #6332

debugger: fix stuck debugger when debuggee exits #6332

reshnm commented Apr 21, 2016 •

edited

Loading

MylesBorins Apr 21, 2016

reshnm Apr 21, 2016 •

edited

Loading

MylesBorins commented Apr 21, 2016

MylesBorins commented Apr 21, 2016

reshnm commented Apr 21, 2016

MylesBorins commented Apr 21, 2016

reshnm commented Apr 22, 2016 •

edited

Loading

reshnm Apr 22, 2016

bnoordhuis Jul 5, 2016

MylesBorins commented May 9, 2016

MylesBorins commented Jun 6, 2016

Trott Jun 6, 2016

reshnm Jun 28, 2016

Trott commented Jun 7, 2016

jjqq2013 commented Jun 28, 2016

MylesBorins commented Jun 28, 2016

MylesBorins commented Jul 5, 2016

MylesBorins commented Dec 29, 2016

jasnell commented Mar 1, 2017

jasnell commented Mar 24, 2017

bnoordhuis commented Mar 26, 2017

bnoordhuis commented Aug 17, 2017

debugger: fix stuck debugger when debuggee exits #6332

debugger: fix stuck debugger when debuggee exits #6332

Conversation

reshnm commented Apr 21, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

MylesBorins Apr 21, 2016

Choose a reason for hiding this comment

reshnm Apr 21, 2016 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Apr 21, 2016

MylesBorins commented Apr 21, 2016

reshnm commented Apr 21, 2016

MylesBorins commented Apr 21, 2016

reshnm commented Apr 22, 2016 • edited Loading

reshnm Apr 22, 2016

Choose a reason for hiding this comment

bnoordhuis Jul 5, 2016

Choose a reason for hiding this comment

MylesBorins commented May 9, 2016

MylesBorins commented Jun 6, 2016

Trott Jun 6, 2016

Choose a reason for hiding this comment

reshnm Jun 28, 2016

Choose a reason for hiding this comment

Trott commented Jun 7, 2016

jjqq2013 commented Jun 28, 2016

MylesBorins commented Jun 28, 2016

MylesBorins commented Jul 5, 2016

MylesBorins commented Dec 29, 2016

jasnell commented Mar 1, 2017

jasnell commented Mar 24, 2017

bnoordhuis commented Mar 26, 2017

bnoordhuis commented Aug 17, 2017

reshnm commented Apr 21, 2016 •

edited

Loading

reshnm Apr 21, 2016 •

edited

Loading

reshnm commented Apr 22, 2016 •

edited

Loading