search

wankdanker / node-discover

Automatic and decentralized discovery and monitoring of nodejs instances with built in support for a variable number of master processes, service advertising and channel messaging.
229 stars 59 forks source link

Crashes if no network adaptor alive (if using multicast) #8

Closed bigmonkeyboy closed 9 years ago

bigmonkeyboy commented 9 years ago

If (for some reason) the network adaptor is unavailable at start and you are using multicast then the module dies horribly with a [addMembership ENODEV] error.

Should be handled in some way - either emit the error back up - or retry in some way.

wankdanker commented 9 years ago

Yikes. This all happens in the constructor, yet it's happening during the nextTick. So, we can't throw and there is not callback to pass the error to. This will take some restructuring to fix.

bigmonkeyboy commented 9 years ago

yup :-) not ideal - but you could add a try catch inside that nextTick - and self.emit the error, then in discover.js pass it on upwards again

    self.broadcast.on("error", function (error) {
        self.emit("error", error);
    });

At least then we can catch it at the app level - even if we have to then destroy the instance and recreate it.

wankdanker commented 9 years ago

Good idea. That makes me think of some other options as well. Including the obvious - add a callback to the constructor. I think I'll use some combination of the two. But I don't necessarily think it's a bad idea if the whole thing blows up if nobody is listening for errors. I'd rather it not work at all than have the app think things are working when they really aren't.

I'm surprised that the self.socket.bind() call doesn't throw an error if there is something up with the network adapter. I need to create an environment for me to experiment with this.

wankdanker commented 9 years ago

I've pushed a branch with some changes. If you could test it out that would be great. If you install it with the below command and then run test.js in the examples directory, hopefully the error message should have bubbled up.

npm install git://github.com/wankdanker/node-discover.git#add-initialize-callback

If this works, I'll merge it and push a new version to npm.

bigmonkeyboy commented 9 years ago

Hi yes - seems to work fine for me. On a related note - the bind error can be provoked by creating a existing listener bound to that port - eg using socat

socat -u udp-recv:12345 -

and can be caught with a self.socket.on("error", down in network.js (and then bubbled up...)

wankdanker commented 9 years ago

Released v0.0.14 to npm.