debugger: fix stuck debugger when debuggee exits by reshnm · Pull Request #6332 · nodejs/node · GitHub

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.

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.

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

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

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

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

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

On master without these changes, the timeout is hit.

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.

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

@Trott any thoughts on this?

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

Done

Pinging @indutny on the C++ changes.

+1

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

/cc @nodejs/diagnostics

ping @nodejs/diagnostics @bnoordhuis

might make sense to fix this in LTS

Does this need to stay open?

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

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.

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

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

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

+                fixture
+              ];
+              const TEST_TIMEOUT_MS = 4000;

debugger: fix stuck debugger when debuggee exits by reshnm · Pull Request #6332 · nodejs/node · GitHub

Uh oh!

Uh oh!

debugger: fix stuck debugger when debuggee exits - #6332

debugger: fix stuck debugger when debuggee exits#6332reshnm wants to merge 3 commits intonodejs:masternodejs/node:masterfrom reshnm:fix_stuck_debuggerCopy head branch name to clipboard

Conversation

reshnm commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

MylesBorins Apr 21, 2016

Choose a reason for hiding this comment

Uh oh!

reshnm Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Apr 21, 2016

Uh oh!

MylesBorins commented Apr 21, 2016

Uh oh!

reshnm commented Apr 21, 2016

Uh oh!

MylesBorins commented Apr 21, 2016

Uh oh!

reshnm commented Apr 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reshnm Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Jul 5, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented May 9, 2016

Uh oh!

MylesBorins commented Jun 6, 2016

Uh oh!

Trott Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

reshnm Jun 28, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 7, 2016

Uh oh!

jjqq2013 commented Jun 28, 2016

Uh oh!

MylesBorins commented Jun 28, 2016

Uh oh!

MylesBorins commented Jul 5, 2016

Uh oh!

MylesBorins commented Dec 29, 2016

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

bnoordhuis commented Mar 26, 2017

Uh oh!

bnoordhuis commented Aug 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

debugger: fix stuck debugger when debuggee exits#6332
reshnm wants to merge 3 commits into
nodejs:masterfrom
reshnm:fix_stuck_debugger

reshnm commented Apr 21, 2016 •
edited
Loading

reshnm Apr 21, 2016 •
edited
Loading

reshnm commented Apr 22, 2016 •
edited
Loading