search

FAForever / java-ice-adapter

A P2P connection proxy for Supreme Commander: Forged Alliance using ICE
MIT License
5 stars 12 forks source link

Bugfix/#26 remove gathering timeout #27

Open BenKluwe opened 4 years ago

BenKluwe commented 4 years ago

Removed gathering timeouts and replaced check for completeness of agent.getState() with agent.getState().isOver(). Following the connectivity establishment the result is logged and if the state is either FAILED or TERMINATED then the thread exits (as per before).

To clarify why both timeouts of the gathering process were removed: the first timeout was preventing the agent from finishing gathering candidates, which is guaranteed to finish. The second timeout was unnecessary and could cause connectivity establishment failure because

  1. the transport protocol used is tcp i.e. guaranteed packet delivery
  2. If the peer takes longer than 6 seconds to gather candidates, the connection is reinitiated resulting in the potential for a loop.
Geosearchef commented 4 years ago

I'll have to check about isOver(), but that sounds nice. I don't think removing the timeouts is a good idea though. I guess you're doing that as gathering still takes up to a minute on your machine? We might be able to take a look at ice4j and figure out/fix it there.

About not timing out gathering candidates, in what way is that guaranteed to finish? That check was added to detect it not finishing and then restart the gathering process with less harvesters, first time no host, second time no srflx, so on the third attempt only the relay candidate should be found which solves the issue of not finishing gathering for lots of people.

About the second timeout, there is no guarantee an ICE message/candidates package will arrive at the other side, as the server might drop it or you may even have lost connection to it. Removing the timeout would effectively break the ability to reconnect.

BenKluwe commented 4 years ago

based on some comments on the discord technical help channel, it seems this is happening not only on my machine.

Ok, so using less harvesters on multiple retries sounds great, but the reason that the gathering is taking so long is it's network adapter that times out after a long time (30,40,50 seconds) and not the method by which gathering happens. Moreover, the gathering process is guaranteed to finish because at some point the underlying connections that ice4j creates will time out or complete (or other failure), thus completing the gathering phase.

Maybe the best compromise is to count the number of active (i.e. with ip address allocated) network adapters between two peers and add a few seconds timeout for each for both peers? Alternatively, how about measuring and uploading/recording the gathering and component creation timings for analysis?

For the second timeout: Ok, I went through the logic again and mistakenly assumed that the connection is established directly between peers, but this is before ICE is finished. Makes sense to keep that timeout then.

EDIT: looks like the ice4j implementation has been updated. Last time I tried to use the ALLOWED_INTERFACES and ALLOWED_ADDRESSES (https://github.com/jitsi/ice4j/blob/38e7b9226d6e1e790fe3b4ccacfeca64e9c2ed71/doc/configuration.md) system properties they didn't work, maybe they do now...