search

paullouisageneau / libplum

Multi-protocol Port Mapping client library
Mozilla Public License 2.0
39 stars 5 forks source link

Currently noprotocol always reports success even when it fails, is this optimum? Also, it gives up after trying all three and quits... #34

Open oviano opened 3 weeks ago

oviano commented 3 weeks ago

Would it not be better to continue cycling through all three protocols indefinitely, and only stopping (as it does now) when it succeeds with one?

I'm thinking of the scenario where this code is running somewhere as a long-running service, and the user is fiddling with their router settings, turning UPnP on and off etc. It's quite possible this could result in it ending up stuck on noprotocol, and it would never detect UPnP/PCP/NAT-PMP being enabled.

I think it would be more robust to make the following three changes:

  1. Reduce the client recheck period to 60s (5 minutes is pretty long)
  2. Allow noprotocol to return failure if it can't map (i.e. local/remote addresses are different)
  3. Instead of exiting the client after failing noprotocol, just loop back to the first protocol.

If you think this might not be what is desired for all scenarios, we could make make the recheck period configurable (via plum_config_t) and likewise a flag to enable or disable "eternal cycling".

Happy to make a PR for these changes if you agree. I already have them made for my own use, but without being configurable.

past-due commented 2 weeks ago

If you think this might not be what is desired for all scenarios, we could make make the recheck period configurable (via plum_config_t) and likewise a flag to enable or disable "eternal cycling".

At least for our usage, we would definitely want this to be configurable at a minimum. (We currently rely on the ability to handle any failure with our own logic.)