Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default behaviour with multiple NICs is not good #109

Open
djcsdy opened this issue Jun 2, 2021 · 0 comments
Open

Default behaviour with multiple NICs is not good #109

djcsdy opened this issue Jun 2, 2021 · 0 comments

Comments

@djcsdy
Copy link

djcsdy commented Jun 2, 2021

Summary:

  • Unless it causes some bad side effect, I think you should make the explicitSocketBind option default to true, and preferably remove it altogether.
  • If my first suggestion isn't an option for some reason, please consider adding socket.setMulticastInterface(iface) to the addMembership function in lib/index.js (requires node >=8).

Judging by some comments in the code and documentation, I think you already know that node-ssdp doesn't behave very well when there are multiple NICs on the system. Unfortunately this is a more common scenario than you might think because many systems, especially developer systems, have virtual NICs for VMs and the like. For example my Windows system has a virtual NIC that is used for WSL and Hyper-V.

The problem is that, although node-ssdp creates a socket for each interface, by default the sockets are not bound to any particular interface. Normally when you send a packet through such a socket, the OS decides which NIC to route the packet through based on the destination IP address. But multicast addresses are associated with every interface, so the OS can choose to send a multicast packet through any NIC essentially at random - the behaviour is undefined. Letting the OS choose here isn't really a viable option because the code will work as expected in some cases but not in others and there will be no logical reason for it.

On Windows, as far as I can tell, in the above circumstances the OS always sends multicast packets to the first available interface, which in my case is the WSL/Hyper-V virtual NIC, so the only SSDP services node-ssdp can find by default for me are the ones hosted on localhost.

I'm not sure what the behaviour of other OSes is. The best case is that the OS sends multicast packets over every interface, in which case the behaviour of node-ssdp is undesirable because it creates one socket per interface, so node-ssdp would send duplicate packets on every interface (I don't think any OS would actually do this, but who knows). The worst case is that the OS picks one interface - the same interface for every socket, and by Murphy's Law probably not the one you wanted - in which case node-ssdp will send duplicate packets on one interface, more or less chosen at random, and ignore the others.

The workaround appears to be to set the explicitSocketBind option. But this doesn't make sense either, because why would correct behaviour be an option, and why would it be off by default? Since node-ssdp creates a socket for every interface, it should be safe to bind that socket to its associated interface. Does this option cause some bad side effect I'm not aware of?

I notice that there is one place in the code where an interface is specified explicitly: https://github.com/diversario/node-ssdp/blob/v4.0.1/lib/index.js#L264. But the addMembership function only tells the socket to listen for multicast requests on that particular interface. It doesn't tell the socket where to send multicast requests.

There is an equivalent function setMulticastInterface, which tells the socket where to send requests, so if for some reason you don't want to bind the socket to a specific interface you might consider adding a call to setMulticastInterface(iface) above the call to addMembership(self._ssdpIp, iface). This does require node 8 though, and I notice that you appear to have deliberately maintained support for older node versions.

I was going to make a PR for this but I wasn't sure if there was some good reason for the existence of the explicitSocketBind option, or which of the above approaches you'd prefer. So I wrote this long rambling explanation instead. Let me know if you'd like me to create a PR. Either way it should be a one line change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant