dgram: make .send cb act as an "error" event handler. #7738

chrisdickinson · 2014-06-05T20:07:47Z

This allows users to provide a callback that handles potential
errors resulting from a socket.send call. The original behavior
of emitting the error event on the socket is preserved.

Fixes #4846.

bnoordhuis · 2014-06-06T16:44:37Z

lib/dgram.js

+
+        if (self.listeners('error').length) {
+          self.emit('error', ex);
+        }


Convention is to drop the quotes for one-line if statements.

bnoordhuis · 2014-06-06T20:40:22Z

LGTM, no comments.

An observation about the review process: it's usually easier when you push fix-up commits to your branch rather than squashing straight away.

When you squash, GH tends to hide comments from previous review rounds and that sometimes makes it difficult to track the flow of the discussion. Once the review is done however, you can squash and land at will.

sam-github · 2014-06-20T21:36:11Z

Changes API, but introduces no documentation!

tjfontaine · 2014-06-23T17:19:01Z

lib/dgram.js

+        callback(ex);
+
+        if (self.listeners('error').length)
+          self.emit('error', ex);


Isn't this redundant, can't we just rely upon the return value from self.emit to determine if this was handled?

If we emit and there's no error listener, the error will be thrown. This check explicitly prevents that if there's a callback.

right -- that part of the api -- sigh sorry

tjfontaine · 2014-06-23T17:19:35Z

I concur with @sam-github that we should clarify here where and when errors will be handled in this way

trevnorris · 2014-06-24T20:35:29Z

Just to clarify, missing changes to the docs is what's holding up this PR?

This allows users to provide a callback that handles potential errors resulting from a `socket.send` call. The original behavior of emitting the error event on the socket is preserved. Fixes nodejs#4846.

mcollina · 2015-05-21T00:13:18Z

Putting some more though on this one, I still think that if there is a callback, then the error should not be emitted, as they will have to 'trap' that error and ignore it, causing the exact same issue described above.
A safe way to keep backward compatibility might be to check the callback.length, as if it has an argument then we can assume the application is going to handle the error. What do you think?

sam-github · 2015-05-22T16:59:18Z

I think we should break compatibility in node v3.x, or else have a different send that does the right thing.

mcollina · 2015-05-22T17:27:50Z

I agree with @sam-github. Do you think it might make sense to send a PR to io.js, or I should wait after the merge happens?

sam-github · 2015-05-22T19:32:59Z

I don't think its worth waiting, and the io.js repo is going to move, to my knowledge, to the new org.

@chrisdickinson

Modifies the dgram send() method to not emit errors when a DNS lookup fails if there is no callback. Given that the same UDP socket can be used to send messages to different hosts, the socket can be reused even if one of those send() fails. This slightly changes the behavior of a stable API, so that it behaves as users would expect to. This is based of @chrisdickinson nodejs/node-v0.x-archive#7738. Discussion in nodejs/node-v0.x-archive#4846.

@chrisdickinson

Modifies the dgram send() method to not emit errors when a DNS lookup fails if there is no callback. Given that the same UDP socket can be used to send messages to different hosts, the socket can be reused even if one of those send() fails. This slightly changes the behavior of a stable API, so that it behaves as users would expect to. This is based of @chrisdickinson nodejs/node-v0.x-archive#7738. Discussion in nodejs/node-v0.x-archive#4846.

@chrisdickinson

Modifies the dgram send() method to not emit errors when a DNS lookup fails if there is no callback. Given that the same UDP socket can be used to send messages to different hosts, the socket can be reused even if one of those send() fails. This slightly changes the behavior of a stable API, so that it behaves as users would expect to. This is based of @chrisdickinson nodejs/node-v0.x-archive#7738. Discussion in nodejs/node-v0.x-archive#4846.

@chrisdickinson

Modifies the dgram send() method to not emit errors when a DNS lookup fails if there is no callback. Given that the same UDP socket can be used to send messages to different hosts, the socket can be reused even if one of those send() fails. This slightly changes the behavior of a stable API, so that it behaves as users would expect to. This is based of @chrisdickinson nodejs/node-v0.x-archive#7738. Discussion in nodejs/node-v0.x-archive#4846.

This allows users to provide a callback that handles potential errors resulting from a `socket.send` call. The original behavior of emitting the error event on the socket is preserved. Fixes: nodejs/node-v0.x-archive#4846 PR-URL: nodejs/node-v0.x-archive#7738 PR-URL: #1796 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Modifies the dgram send() method to not emit errors when a DNS lookup fails if there is a callback. Given that the same UDP socket can be used to send messages to different hosts, the socket can be reused even if one of those send() fails. This slightly changes the behavior of a stable API, so that it behaves as users would expect to. This is based on nodejs/node-v0.x-archive#7738, which landed in 77266d7. Fixes: nodejs/node-v0.x-archive#4846 Refs: nodejs/node-v0.x-archive#7738 PR-URL: #1796 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

cjihrig · 2015-06-05T14:38:12Z

Closing this. The fix can be back ported from nodejs/node@77266d7 and nodejs/node@90daf87.