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?

[indutny]

@tjfontaine yay or nay for 0.12?

[tjfontaine]

I'm +1 on the idea, making sure we preserve the existing emission behavior for those with handlers installed

[mcollina]

The current workaround is this: https://github.com/mcollina/node-coap/blob/master/lib/agent.js#L54-L58.

sock.on('error', function(err) {
  // we are skipping DNS errors
  if(err.code !== 'ENOTFOUND')
    that.emit('error', err)
})

I think we cannot support it and stop emitting the error: it's good habit to have a listener on the error event of a socket.

Here is the best behavior we can support:

1. if there is no callback in the send method, then emit as of today
2. if there is a callback in the send method, report the error there.

[jasnell]

@bnoordhuis @indutny ... do we still want to do something with this one?

[mcollina]

@jasnell there is a PR here: #7738

[cjihrig]

This is now fixed on the io.js next branch in nodejs/node@77266d7 and nodejs/node@90daf87.

[jasnell]

Do we want to backport?
On Jun 5, 2015 7:36 AM, "Colin Ihrig" [email protected] wrote:

This is now fixed on the io.js next branch in
77266d7fadd8dfefb107ccb1e3fe97f9620f1288 and
90daf870a00305dcb4b7bf474d8eafaed0117242.

—
Reply to this email directly or view it on GitHub
#4846 (comment).

[cjihrig]

It is a semver major change, so if so, it needs to go into master.

[jasnell]

I'd say no backport then. We can pick it up later in the converged stream.
On Jun 5, 2015 7:39 AM, "Colin Ihrig" [email protected] wrote:

It is a semver major change, so if so, it needs to go into master.

—
Reply to this email directly or view it on GitHub
#4846 (comment).

[cjihrig]

Works for me.

[mcollina]

Great!!