Catch and emit error on socket.addMembership

[huanvu]

socket.addMembership throws an error on network issues (no network). This commit catches and emits the error so that the error can be handled elsewhere instead of crashing the library.

[pfrazee]

We've hit this in Beaker (https://github.com/beakerbrowser/beaker/issues/323). @mafintosh can you merge?

[mafintosh]

6.1.1

[pfrazee]

Thanks!

On Tue, Mar 7, 2017 at 1:17 AM, Mathias Buus notifications@github.com wrote:

6.1.1

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mafintosh/multicast-dns/pull/34#issuecomment-284641661, or mute the thread https://github.com/notifications/unsubscribe-auth/ABNhU89PY0-2tZsSo_eOFPE-ajzXGaDVks5rjQSMgaJpZM4KEd3w .

[thom-nic]

Hey! I think this is what I was hitting in watson/bonjour#38 ❤️ Time to upgrade...

Edit: 💔 actually this patch was the cause of my problem. What you have now is a socket that's created, an emit('error') added and a bind() call all inside a constructor, so there's no way for me to reliably handle the error event... and an unhandled error event == process 💀

I can do the following:

const mdns = require('multicast-dns');
const myMdns = mdns(opts);
myMdns.on('error', err => console.warn("please don't crash my nodejs process"));

However I don't think there's any guarantee that my error handler will definitely be added before a socket.bind() emits an error. I think the proper way to handle this would for the constructor to return an instance with a bind() or listen() method, so you can do the following:

const myMdns = mdns(opts); // socket is not yet opened
myMdns.on('error', err => console.warn("please don't crash my nodejs process"));
myMdns.listen(); // now we can handle events

Obv that is a breaking API change, however.

See: https://nodejs.org/api/events.html#events_error_events I hope I'm preaching to the choir when I say you need to be really careful about when and how you emit error events due to the fact they can crash your process (I see it done in two places in that constructor) and document that fact really well. I don't see any mention in the README that the returned instance can emit error events.

Happy to open a new issue to address this, just let me know and I'll move the discussion.

[thom-nic]

If we would like to fix this in a backward-compatible way, possibly:

const myMdns = mdns({autoOpen: false}); // socket will not be opened automatically, defaults to true for compat
myMdns.on('error', err => console.warn("please don't crash my nodejs process"));
myMdns.listen(); // new method to open the socket, if autoOpen: false option was given in ctor