Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it easier to debug unhandled error events #3270

Closed
binarykitchen opened this issue Oct 8, 2015 · 12 comments
Closed

Make it easier to debug unhandled error events #3270

binarykitchen opened this issue Oct 8, 2015 · 12 comments
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@binarykitchen
Copy link

We all hard-working developer fear this infamous error:

events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: This socket is closed.
    at Socket._writeGeneric (net.js:638:19)
    at Socket._write (net.js:692:8)
    at doWrite (_stream_writable.js:292:12)
    at writeOrBuffer (_stream_writable.js:278:5)
    at Socket.Writable.write (_stream_writable.js:207:11)
    at Socket.write (net.js:616:40)
    at Socket.ondata (_stream_readable.js:521:20)
    at emitOne (events.js:82:20)
    at Socket.emit (events.js:169:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:110:10)
    at Pipe.onread (net.js:521:20)

They are a pain to investigate. Can we make it somehow easier to debug those?

For example unhandled error events coming from sockets could at least include a better error message saying what kind of socket it is. And what listeners are attached to it. That would help enormously.

@bnoordhuis bnoordhuis added the feature request Issues that request new features to be added to Node.js. label Oct 8, 2015
@bnoordhuis
Copy link
Member

Pull requests welcome. Better error messages would be good but it depends on how it's implemented.

@binarykitchen
Copy link
Author

Ok, will try.

Can you tell me what's the easiest way to make changes in the net.js file, compile only that change and to run a single test case (/test/parallel/test-net-pingpong.js) without affecting anything else?

Because running all tests take more than one minute but I only want to work on a small piece.

@bnoordhuis
Copy link
Member

You always have to rebuild after making changes to files in lib/ and src/. To check only that one test, run python tools/test.py parallel/test-net-pingpong.

The test runner accepts wildcards, by the way. python tools/test.py parallel/test-net-* would run all parallelizable net tests.

@binarykitchen
Copy link
Author

@bnoordhuis Got it, thanks. But one thing is weird. I added some debugging information such as console.log('test') in test/parallel/test-net-pingpong.js but they are never shown when running the test for it:

$ python tools/test.py parallel/test-net-pingpong
[00:00|% 100|+   1|-   0]: Done  

How can I make the output visible within the test execution?

@akreienbring
Copy link

I'm also seeing a similar error when trying to access mongodb with mongoose. What is the reason for this? And even more important: How to prevent node from crashing? I already catch all errors (try.. catch) and the server still crashes... :-(
My node version is 0.12.7

@bnoordhuis
Copy link
Member

@binarykitchen Run the test standalone: out/Release/node test/parallel/test-net-pingpong.js.

@binarykitchen
Copy link
Author

@akreienbring try ...catch won't work here because of the async nature of sockets, being run in separate "threads" I think.

@binarykitchen
Copy link
Author

@bnoordhuis Ok, I've got an idea and have tested it successfully. In /lib/net.js on line 639 we could write something like this:

  if (!this._handle) {
    this._destroy(new Error('This socket is closed. Unable to write data ' + data.toString(encoding)), cb);
    return false;
  }

Then in /test/parallel/test-net-pingpong.js I added delete socket._handle somewhere along the lines and got this better error message:

Error: This socket is closed. Unable to write data PONG

What's missing is a small function to limit data.toString(encoding) up to i.E. 10 chars. Does such a summarize function already exist in node core?

Anyway, I believe that revealing the first 10 chars of data can help developers to locate the socket issue.

Let me know your thoughts and I ll produce a concrete PR.

@bnoordhuis
Copy link
Member

data.toString(encoding, 0, 10), assuming data is a buffer? Two issues I see:

  1. It will mangle partial character sequences. E.g. Buffer('\u00e9').toString('utf8', 0, 1) does not produce a valid UTF-8 string.
  2. You would need to escape characters outside of the printable range, or you may end up clobbering the terminal.

@binarykitchen
Copy link
Author

@bnoordhuis Right, I am aware of that. Igniting a discussion here to collect best ideas. Give me some time.

@Others Suggestions welcome

@Shadowstep33
Copy link

Is there any way to wait for the buffer to have finished before running this._destroy or would that defeat the purpose entirely?

Any way promises could be of use here?
http://www.html5rocks.com/en/tutorials/es6/promises/

@Trott
Copy link
Member

Trott commented Apr 12, 2017

This conversation seems to have run its course. I'm going to close it. Feel free to re-open, comment here, or open a new issue if this is still something worth discussing more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

5 participants