Do not include network aliases (identified by same MAC)

htool commented 1 year ago

Starting a listener on a network alias fails, so do not include them in allAddresses function.

htool commented 1 year ago

Solves https://github.com/coapjs/node-coap/issues/355

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 5175877906

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.1%) to 91.795%

Files with Coverage Reduction	New Missed Lines	%
lib/server.ts	2	86.93%
<!--	Total:	2		-->

Totals
Change from base Build 5048002328:	-0.1%
Covered Lines:	2860
Relevant Lines:	3073

💛 - Coveralls

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 9362935462

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
45 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.2%) to 91.676%

Files with Coverage Reduction	New Missed Lines	%
lib/outgoing_message.ts	1	97.93%
lib/server.ts	44	86.56%
<!--	Total:	45	-->

Totals
Change from base Build 5048002328:	-0.2%
Covered Lines:	2889
Relevant Lines:	3106

💛 - Coveralls

Apollon77 commented 1 year ago

Additionally to the change request : how this detects network aliases? Is it secured that the main entry always comes before the Alia's in the list? No other option to detect them? Could I see such a list with aliases included please?

htool commented 1 year ago

One of them gets included. No other way to see other than matching macs.

htool commented 1 year ago

Good catch on my copy mistake.

Scenario is when having multiple ips on the same interface. The interface can only have 1 listener.

JKRhb commented 1 year ago

Can you maybe post a code snippet or script for reproducing the error? I tried adding an alias to a network interface (using something along the lines of ip link set foo alias bar) but the original code seems to still work fine afterward. It is not unlikely that I made an obvious mistake here, though.

htool commented 1 year ago

Ip addr add to add a second ip to an interface. It's not alias but additional ip on same interface.

Apollon77 commented 1 year ago

Hm .. but why this do not work? Do we not listen on that up, so multiple listeners should work. Which error exactly you get?

htool commented 1 year ago

Adding second ip to existing wifi interface: $ sudo ip addr add 192.168.1.125 dev wlp64s0

allAddresses will list both IPs, while being the same interface.

Error: addMembership EADDRINUSE
    at Socket.addMembership (node:dgram:849:11)
    at /home/hans/.signalk/node_modules/coap/dist/lib/server.js:207:34
    at Array.forEach (<anonymous>)
    at Socket.<anonymous> (/home/hans/.signalk/node_modules/coap/dist/lib/server.js:206:58)
    at Socket.onListening (node:dgram:252:7)
    at Socket.emit (node:events:513:28)
    at startListening (node:dgram:181:10)
    at node:dgram:366:7
    at processTicksAndRejections (node:internal/process/task_queues:84:21)
    at runNextTicks (node:internal/process/task_queues:65:3)
    at processTimers (node:internal/timers:499:9) {
  errno: -98,
  code: 'EADDRINUSE',
  syscall: 'addMembership'
}

JKRhb commented 1 year ago

Thank you for the stack trace! I think the problem here is actually that the server tries to join the same multicast group twice. For some reason, it iterates over the list of IP addresses instead of a list of network interfaces. Therefore, if you add another IP address (which appears to be used as an identifier here), calling Socket.addMembership() will fail.

Maybe we could try replacing the addresses as identifiers with os.getNetworkInterfaces() to get the proper interface names..? I need to have a closer look into this later.

JKRhb commented 1 year ago

Since allAddresses is apparently only used for getting the addresses as interface identifiers for the multicast configuration, I think the approach proposed by @htool is actually an appropriate fix. Maybe we could rename the allAddresses method or add some documentation to make its (de-facto) purpose a bit clearer.

htool commented 2 months ago

@Apollon77 What still needs to be done to move this forward? I don't see how I can further influence it.

Apollon77 commented 2 months ago

@htool Maybe see the last comment from @JKRhb as reveiw feedback:

Maybe we could rename the allAddresses method or add some documentation to make its (de-facto) purpose a bit clearer.

And rename method or add acomment describing the behavior and why? Then I think we can merge here.

@JKRhb any other opinion?

htool commented 2 months ago

I've added a comment to explain why the repeating MAC check is there.

coapjs / node-coap