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

Bugfix/#26 remove gathering timeout #27

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

BenKluwe
Copy link

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.

BenKluwe and others added 4 commits March 26, 2020 09:31
Tray icon / Info Window fixes
…d does not fail on installing oraclejdk8
… because

1. the rpc server uses tcp and therefore packet delivery is guaranteed
2. if the peer takes longer than 6 seconds to gather candidates then the local timeout will prevent connectivity establishment
@Geosearchef
Copy link
Member

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
Copy link
Author

BenKluwe commented Mar 30, 2020

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...

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

Successfully merging this pull request may close these issues.

2 participants