skipping exceptions throws by dgram on dns lookup failure

[danivek]

If you create a udp4 socket and send some data over it to an hostname that cannot be resolved, unfortunately, dgram from node (<=0.12.x) will throw a top-level exception which kills the entire app.

This is a workaround to avoid this exception.
This issue is closed on recent node version (see nodejs/node-v0.x-archive#4846 for more information) but not on branch 0.12.x and earlier

[cthayer]

According to the thread you mentioned, to skip the DNS errors, this should be wrapped in a check for the ENOTFOUND error code.

if (err.code !== 'ENOTFOUND')

[danivek]

This is not the only error i have. With different version of OS, i have these error code ESRCH (centos7), EAI_AGAIN (ubuntu), etc...
I think it depend on how dns lookup is manage on OS

[cthayer]

I think I understand. You're not trying to suppress the errors, you're trying to get them emitted up to a level that you can manage them at?

[danivek]

Exactly. I'm not trying to suppress the errors. In old version of node, it appears that even if you're using the dgram socket as a send-only connection, you still need to have an error handler defined on the socket listener. If there's not an error listener on the socket, you'll get an unhandled exception propagated to the root. With this error listener, we avoid the top level exception. The throwned exception in dns.js has been fix in recent node version only.

[cthayer]

Makes sense. Can you modify the comment to reflect this? Then we just need a test to check that the error event is emitted and I'll be happy to merge this.

[cthayer]

I agree that we should attach an error handler on the socket.

However, I thinking that suppressing DNS error events would be something that can/should be configurable (maybe with the default being to suppress the DNS error events).

These errors would still be accessible via the callback passed to this.client.send() even if they are being suppressed in the error handler.

[danivek]

I try to make a unit test that make sense on all version of node. unfortunately, UDP DNS errors only happen on node <= 0.12.x... I'm not sure if the conditonnal on node's version is correct and is the best way in mocha test...

[cthayer]

I don't think there's a fundamentally different way to run tests for specific versions of node.

Did you try running the test on node 4.x?

According to the thread you referenced in the initial pull request, it seems as if the solution was to stop firing the 'error' event only when a callback was present. So my expectation would be that the 'error' event should still be fired when no callback is present.

However, in this transport, send() is always being called with a callback so not seeing the error event would make sense in node versions > 12.

Perhaps the default callback that is passed to send() should be modified to emit the error event when there is an error? Then the behavior should be the same on all node versions.

[danivek]

Yes, I try running the test on node 4.x, the error event is never emitted and there is no error in callback , because no error happened on the UDP socket with this version.

[cthayer]

So I did some testing in node 4.1.0 and the dns error is still happening on the socket, but there are two layers of abstraction on the callback passed to send() so the error was getting lost and effectively suppressed.

It's a pretty easy fix so I'm going to go ahead and accept this PR and then make the minor tweaks to make the callbacks and error handling work the same in all node versions.

[cthayer]

version 0.1.0 is release in NPM with these changes to socket error handling

[cthayer]

After doing some more testing, I think it is best for the transport to return the error object rather than emit an error event.

Since the transport always passes a callback to send() and winston.log() always passes a callback to the transport's log() the most consistent behavior is to return the error object rather than emit an error event.

So I'm going to make another release with this change.

In version 0.1.1, you will do the following to get the error:

var logger = createLogger(port, {host: 'unresolvedhost'});

logger.log('info', 'hello world', {stream: 'sample'}, function (err) {
    if (err) {
        // do something
    }
});

This way an error event is never emitted in any version of node and logging to this transport will never crash your application regardless of wether you care about errors or not (which I think is the way this transport should behave).

[cthayer]

version 0.1.1 is released now

[danivek]

Great. Thanks for your help !