DebuggerAgent: unhandled socket error event

[3y3]

debugger_agent doesn't handle correctly socket closing.

Test case:

node --debug test

Test.js

var spawn = require('child_process').spawn;

function Debugger(port) {
  var connection = require('net').createConnection(port)
    .on('error', console.error.bind(console))
    .on('close', process.exit.bind(process))
    .setEncoding('utf8');

  process.on('exit', connection.end);
}

var _debugger = spawn('node', ['-e', '(' + Debugger + ')(5858)']);
setTimeout(_debugger.kill.bind(_debugger), 1000);

After fix this issue by:

diff --git a/deps/debugger-agent/lib/_debugger_agent.js b/deps/debugger-agent/lib/_debugger_agent.js
index 680c5e9..13f2a9f 100644
--- a/deps/debugger-agent/lib/_debugger_agent.js
+++ b/deps/debugger-agent/lib/_debugger_agent.js
@@ -97,6 +97,7 @@ function Client(agent, socket) {
   this.on('data', this.onCommand);

   var self = this;
+  this.socket.on('error', function() {});
   this.socket.on('close', function() {
     self.destroy();
   });

I receive next error:
Assertion failed: (err) == (0), file src\agent.cc, line 164
UV_EBUSY

So, I'm incompetent to fix uv errors, therefore I opened this issue.
I think this is for @indutny

[JacksonTian]

I don't reproduce it. But i get the same issue(maybe):

$ node-debug index.js
Node Inspector is now available from http://127.0.0.1:8080/debug?port=5858
Debugging `index.js`

Debugger listening on port 5858
Assertion failed: ((err) == (0)), function Stop, file ../src/debug-agent.cc, line 155.

when close browser, the assertion error was threw.

the index.js:

console.log('hello world');

iojs version: 1.6.5

[dickeylth]

encounted the same problem on both webstorm node.js debugger and node-inspector.

[bnoordhuis]

I can confirm the issue but a proper fix will probably have to wait until #1159 is merged.

The problem is that the debugger event loop still has open libuv handles by the time it gets shut down. There is currently no good way to force-close handles (at least, not without introducing new bugs) until #1159 adds the necessary infrastructure.

[indutny]

@bnoordhuis hm... have this handles creeped out of the Environment? I believe I introduced some sort of class for this thing.

[yjhjstz]

(gdb) bt
#0 uv_tty_init (loop=0x3428740, tty=0x7fffe40483e8, fd=2, readable=0) at ../deps/uv/src/unix/tty.c:42
#1 0x00000000010617af in node::TTYWrap::TTYWrap (this=0x7fffe4048380, env=0x7fffe404b4d0, object=..., fd=2, readable=false) at ../src/tty_wrap.cc:188
#2 0x00000000010616f6 in node::TTYWrap::New (args=...) at ../src/tty_wrap.cc:178
#3 0x0000000000aac9ff in v8::internal::FunctionCallbackArguments::Call (this=0x7ffff63c3ba0,
f=0x10615be node::TTYWrap::New(v8::FunctionCallbackInfo<v8::Value const&)>) at ../deps/v8/src/arguments.cc:33
#4 0x0000000000ae5fad in v8::internal::HandleApiCallHelper (args=..., isolate=0x7fffe40008c0) at ../deps/v8/src/builtins.cc:1144
#5 0x0000000000ae067d in v8::internal::Builtin_Impl_HandleApiCallConstruct (args=..., isolate=0x7fffe40008c0) at ../deps/v8/src/builtins.cc:1166
#6 0x0000000000ae0643 in v8::internal::Builtin_HandleApiCallConstruct (args_length=4, args_object=0x7ffff63c3cf0, isolate=0x7fffe40008c0)
at ../deps/v8/src/builtins.cc:1165

Breakpoint 1, uv_loop_close (loop=0x3428740) at ../deps/uv/src/unix/loop.c:65
65 printf("type = %d, loop handle %p\n", h->type, h);
(gdb) p h
$8 = (uv_handle_t *) 0x7fffe40483e8

that is to say, 'tty' don't close, leading to the assert. @bnoordhuis @JacksonTian

[Trott]

I can confirm the issue but a proper fix will probably have to wait until #1159 is merged.

#1159 was subsequently closed in favor of #2133. That issue was active within the last couple weeks, but I don't know how far it is from landing. (Hi, I'm updating old tickets, kthxbai.)

[bnoordhuis]

#2133 needs reviewers. Perhaps it's possible to cherry-pick the handle cleanup code but someone will still need to review that.

See also #5270 for an attempted ad hoc fix using uv_walk().

[Trott]

Given 15 months of dormancy for this issue combined with the --debug/debugger being removed in current Node.js in favor of --inspect, I'm going to close this issue. By all means, if that's premature, comment or (if GitHub allows you to) re-open the issue. (I'm just housecleaning a bit in the issue tracker.)