From bf3e0deae71c765d9a718c9b29ec3bbb7576ae37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Sch=C3=BCnemann?= Date: Thu, 21 Apr 2016 17:40:36 +0200 Subject: [PATCH 1/3] debugger: fix stuck debugger when debuggee exits 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. --- lib/_debug_agent.js | 10 ++++++++++ src/debug-agent.cc | 8 ++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/_debug_agent.js b/lib/_debug_agent.js index cb4684f0eddfac..436d53dbb039d6 100644 --- a/lib/_debug_agent.js +++ b/lib/_debug_agent.js @@ -36,6 +36,10 @@ exports.start = function start() { process.removeListener('SIGWINCH', fn); }); + // Disconnect all currently connected clients. + // Without this, the debugger agent loop will not unblock + // and the debuggee will get stuck. + agent.destroyAllClients(); agent.close(); }; @@ -81,6 +85,12 @@ Agent.prototype.notifyWait = function notifyWait() { this.first = false; }; +Agent.prototype.destroyAllClients = function destroyAllClients() { + this.clients.forEach(function(client) { + client.destroy(); + }); +}; + function Client(agent, socket) { Transform.call(this, { readableObjectMode: true diff --git a/src/debug-agent.cc b/src/debug-agent.cc index e4e0ea061bd30f..2c3fdea3159864 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -128,6 +128,11 @@ void Agent::Enable() { parent_env()->AssignToContext(debug_context); } +static void close_handle(uv_handle_t* handle, void* data) { + if (!uv_is_closing(handle)) { + uv_close(handle, nullptr); + } +} void Agent::Stop() { int err; @@ -149,6 +154,9 @@ void Agent::Stop() { CHECK_EQ(err, 0); uv_close(reinterpret_cast(&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); uv_run(&child_loop_, UV_RUN_NOWAIT); err = uv_loop_close(&child_loop_); From 56d54d35811eb0b515025d79bfa7904fed67ec4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Sch=C3=BCnemann?= Date: Fri, 22 Apr 2016 09:28:28 +0200 Subject: [PATCH 2/3] debugger: added test for stuck debugger 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. --- .../debugger-stuck-regression-fixture.js | 2 + .../test-debugger-stuck-regression.js | 93 +++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100644 test/fixtures/debugger-stuck-regression-fixture.js create mode 100644 test/parallel/test-debugger-stuck-regression.js diff --git a/test/fixtures/debugger-stuck-regression-fixture.js b/test/fixtures/debugger-stuck-regression-fixture.js new file mode 100644 index 00000000000000..32ce0a1c92ede9 --- /dev/null +++ b/test/fixtures/debugger-stuck-regression-fixture.js @@ -0,0 +1,2 @@ +'use strict'; +console.log('debug'); diff --git a/test/parallel/test-debugger-stuck-regression.js b/test/parallel/test-debugger-stuck-regression.js new file mode 100644 index 00000000000000..566cc91deed598 --- /dev/null +++ b/test/parallel/test-debugger-stuck-regression.js @@ -0,0 +1,93 @@ +'use strict'; +const path = require('path'); +const spawn = require('child_process').spawn; +const assert = require('assert'); + +const common = require('../common'); + +const fixture = path.join( + common.fixturesDir, + 'debugger-stuck-regression-fixture.js' +); + +const args = [ + 'debug', + `--port=${common.PORT}`, + fixture +]; + +const TEST_TIMEOUT_MS = 4000; + +function onTestTimeout() { + common.fail('The debuggee did not terminate.'); +} + +const testTimeout = setTimeout(onTestTimeout, TEST_TIMEOUT_MS); + +const proc = spawn(process.execPath, args, { stdio: 'pipe' }); +proc.stdout.setEncoding('utf8'); +proc.stderr.setEncoding('utf8'); + +const STATE_WAIT_BREAK = 0; +const STATE_WAIT_PROC_TERMINATED = 1; +const STATE_EXIT = 2; + +let stdout = ''; +let stderr = ''; +let state = 0; +let cycles = 2; + +proc.stdout.on('data', (data) => { + stdout += data; + + switch (state) { + case STATE_WAIT_BREAK: + // check if the debugger stopped at line 1 + if (stdout.includes('> 1')) { + stdout = ''; + + // send continue to the debugger + proc.stdin.write('c\n'); + state = STATE_WAIT_PROC_TERMINATED; + } + break; + case STATE_WAIT_PROC_TERMINATED: + // check if the debuggee has terminated + if (stdout.includes('program terminated')) { + stdout = ''; + + // If cycles is greater than 0, + // we re-run the debuggee. + // Otherwise exit the debugger. + if (cycles > 0) { + --cycles; + state = STATE_WAIT_BREAK; + proc.stdin.write('run\n'); + } else { + // cancel the test timeout + clearTimeout(testTimeout); + state = STATE_EXIT; + proc.stdin.write('.exit\n'); + } + + } + break; + case STATE_EXIT: + // fall through + default: + break; + } +}); + +proc.stderr.on('data', (data) => { + stderr += data; +}); + +proc.stdin.on('error', (err) => { + console.error(err); +}); + +process.on('exit', (code) => { + assert.equal(code, 0, 'the program should exit cleanly'); + assert.equal(stderr, '', 'stderr should be empty'); +}); From d9d6294e3984570cf2c3e245693cdbe11f0a243e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Sch=C3=BCnemann?= Date: Tue, 28 Jun 2016 09:36:30 +0200 Subject: [PATCH 3/3] debugger: use common.platformTimeout Use common.platformTimout to give ARM devices extra time for test execution. --- test/parallel/test-debugger-stuck-regression.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-debugger-stuck-regression.js b/test/parallel/test-debugger-stuck-regression.js index 566cc91deed598..ad4b0cc58c4e2e 100644 --- a/test/parallel/test-debugger-stuck-regression.js +++ b/test/parallel/test-debugger-stuck-regression.js @@ -16,7 +16,7 @@ const args = [ fixture ]; -const TEST_TIMEOUT_MS = 4000; +const TEST_TIMEOUT_MS = common.platformTimeout(4000); function onTestTimeout() { common.fail('The debuggee did not terminate.');