upnp port mapping #43
Conversation
|
also not sure whatsup with the tests. They all pass but the process does not exit after the tests finish. I tried on master and it has the same problem. |
|
Good idea to check into this (upnp) and see if it can help improve connection rates. Need to double check that there's not a downside, like corporate/university LANs seeing it as bad activity. |
|
@pfrazee I think it's worth trying out since this could improve connections for a large chunk of people -- orgs with high security could just outright disable upnp if they don't want it. |
|
great work @jgautier ! |
|
It seems like there is no unmapping done on close? |
|
@martinheidegger right here is where the unmapping happens on close: https://github.com/mafintosh/discovery-swarm/pull/43/files#diff-168726dbe96b3ce427e7fedce31bb0bcR107 is there an additional place I should add an unmapping? This only works if you properly close/destroy the swarm. If you exit a program without calling destroy on the swarm then the port stays mapped forever. This is actually pretty annoying since now I have tons of ports mapped on my router while doing testing :P. I think the better approach is probably to set the ttl like for an hour or a configurable time and then have a interval timer that renews the port mapping. @pfrazee @Karissa What do you guys think is a good way to test whether this would work well in a corporate/university setting? What do you guys think needs to be done to get this merged? For me I think this is my checklist
One idea for the test would be to import the nat upnp module in the test script and the use it to get the mappings and check that the port was actually opened. I don't know how well this would work because the test would likely fail if you have upnp turned off on your network and I am guessing wherever travis is running the tests also would not have upnp. Other then that the only other way I could test to think of is to mockout the nat-upnp object and verify the functions are being called. Thanks everyone! |
|
@jgautier as for the ttl, it sounds good but perhaps if the process is still running could we renew the ttl? people could have their dats replicating indefinitely. |
That sounds right to me. (I haven't gotten a chance to look at the protocol or your implementation, but--) Is it possible to make the port mappings idempotent so that multiple calls will overwrite the previous instead of accumulating? @mafintosh should definitely take a look at this |
|
🙌 🙏 |
Not sure if this fixes #1 but it does use upnp to open ports. If the router supports it this allows clients behind NATs to talk over TCP to each other. Couple things to point out:
I'm a little unsure about the impliedPort vs publicPort when joining the swarm. I basically made it so if the mapping fails it falls back to the impleidPort logic otherwise sets the publicPort to the port that was mapped.
It has to wait for the 'listen' event before the mapping since the port is not always known until after that event, and I made sure to try to open the port before the join is called (not sure if this part matters).
The ttl for the part mapping is set to 0 which keeps the port open forever which might be annoying to have your router have so many open ports. Maybe a different approach would be to set the ttl to lower and then re-map on an interval. That way the ports will close eventually after the program has exited.
I have not written a test. I am open to writing one just not sure what a good case would be. I tested this manually by running the example with data-swarm-defaults, utp: false and running the 2 swarms behind a NAT.
Thanks!