UDP throws unhandled error, even if callback is provided to `send`.

[jwalton]

If you create a udp4 socket and send some data over it to an invalid hostname (as in the Coffee-Script example below), then the callback will be called to indicate that an error occurred. Unfortunately, the udp socket will ALSO throw a top-level exception which kills the entire app.

Sample:

socket = (require 'dgram').createSocket 'udp4'
#socket.on "error", (err) ->
#  console.log "Error handler: #{err}"

buffer = new Buffer("foo")
callback = (err, bytes) ->
  if err
    console.log("Error sending: #{err}")
  else
    console.log("Sent #{bytes}")
socket.send(buffer, 0, buffer.length, 100, "asdkasd", callback)

Run with "coffee sample.coffee". You'll get the following output:

Error sending: Error: getaddrinfo ENOTFOUND

events.js:71
        throw arguments[1]; // Unhandled 'error' event
                   ^
Error: getaddrinfo ENOTFOUND
    at errnoException (dns.js:37:11)
    at Object.onanswer [as oncomplete] (dns.js:124:16)

And the problem If you uncomment the two lines at the top, and define a "on error" handler, then the callback will be called with the error, and the error handler will also be called, and the program will not terminate (since the socket is never closed.)

I would expect that either the presence of a callback or a defined error handler would prevent killing the program, and not require both?

[jwalton]

Tested on v0.8.21, BTW.

[bnoordhuis]

It's a backwards compatibility thing but I agree it's kind of quirky. I don't mind changing it for v0.10 but the thing is that the rules (i.e. what you have to keep in mind when you use the API) become more complex. Currently it works like this:

1. You need an 'error' event listener.
2. The callback is optional.

That would change to:

1. You need an 'error' event listener...
2. ...unless you pass in a callback...
3. ...but the error event should still be emitted when there's at least one 'error' listener because not doing that would break a lot of existing code.

The actual change is pretty trivial though:

diff --git a/lib/dgram.js b/lib/dgram.js
index 222de73..6787cff 100644
--- a/lib/dgram.js
+++ b/lib/dgram.js
@@ -264,7 +264,8 @@ Socket.prototype.send = function(buffer,
   self._handle.lookup(address, function(err, ip) {
     if (err) {
       if (callback) callback(err);
-      self.emit('error', err);
+      if (!callback || self.listeners('error').length > 0)
+        self.emit('error', err);
     }
     else if (self._handle) {
       var req = self._handle.send(buffer, offset, length, port, ip);

[jwalton]

I obviously vote to change it. :) But, if it doesn't get changed, we should at least call this out explicitly in the documentation (if you have a callback, you also need an error handler, or else the whole app could die in a fire. :) The stack trace for the uncaught exception doesn't tell you anything about where it came from, so tracking this down in a big application with a lot of network IO is not much fun (as I can personally attest to. :P)

[jwalton]

Oh oh! While I'm on the subject, it would be super awesome if, instead of just "getaddrinfo ENOTFOUND", the exception had the name of the domain name that wasn't found in it. This is actually how I eventually tracked this down; by spinning a custom node.js that included the domain name it was looking up, so I had some kind of hint where this was coming from. :)

[hcwiley]

I can also attest to the "tracking this down in a big application with a lot of network IO is not much fun". I have callback function to handle the error, but node crashes anyways since it calls a sys level error. Are there any plans to adopt this?

[isaacs]

I'd really rather not make the change that @bnoordhuis suggested above. The current behavior is consistent with stream.write(chunk, encoding, callback) where the callback gets called, but errors are also emitted on the object.

Add an error event listener on the object. That's all there is to it. Sorry. I know it's wonky. If I could do it over again, I'd say that we just not pass the error to a callback in that way. and only have the foo.on('error', er) handling style, since it's more consistent.

@jwalton, your suggestion about putting the domain name on the exception is great, but needs to go in a separate issue: #5393

[bnoordhuis]

I'm reopening this issue. I've been thinking it over and I feel this needs a little more consideration. In particular:

The current behavior is consistent with stream.write(chunk, encoding, callback) where the callback gets called, but errors are also emitted on the object.

That's true but streams and datagrams are different beasts. The former is a 1-on-1 communication channel, the latter one-to-many; you can't really compare them.

The current API was put together without much thought and it shows. We won't be making radical changes but I think some cleanup is in order and this is one such wart.

[mcollina]

I totally agree with @bnoordhuis. As an example, take CoAP. It has all the mechanisms to reuse the same socket for multiple message exchanges with different hosts.

I think there are two kind of errors: the ones related generally to the socket, and the ones related to the current message being sent. The general errors should go into the socket error event, but the specific errors should not: if sending a message fails for a DNS resolution, the socket is totally fine and can be reused. Moreover, if there were other exchanges going on, the correct behavior with the actual API is to error all of them.

My actual solution is to filter the errors codes in the error handler, but it's bad and everybody in the very same situation will have the same problem.

[Sannis]

@bnoordhuis are there any plans to to this befor 0.12.x?