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

Update ice4j to mainline and off custom fork #46

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

Sheikah45
Copy link
Member

No description provided.

@Geosearchef
Copy link
Member

Have you seen this commit?

Geosearchef/ice4j@52c8e4b

Without that and some reflection hack in the adapter code (not sure if that breaks with a newer version), your relay connections will time out and die after 10 mins.

@Sheikah45
Copy link
Member Author

I played multiple full games beyond 10 minutes with the latest version

@Sheikah45
Copy link
Member Author

Also the lifetime is supposed to be explicitly set from the stun response as well. So using the value returned is proper. As this is the interval requested by the stun server

@Geosearchef
Copy link
Member

If you didn't use a relay candidate, it doesn't matter that it worked for you, it won't work for others who need a relay candidate.

About the lifetime value, in theory that should be correct, in practice I can just tell you it didn't work back when I developed this and the turn server did not act according to documentation.

Do with that what you will, just saying, there's a reason I had to do this and you should test multiple games with multiple people with the new adapter before deploying any business logic changes to the adapter.

@Sheikah45
Copy link
Member Author

I had force relay on for some of the games as well. And yes I plan on testing

@Geosearchef
Copy link
Member

I had force relay on for some of the games as well. And yes I plan on testing

Ok, then it's probably fine.
Just providing some info from when this was developed, as there were a lot of quirks I had to work around.

(If I remember correctly, back in the day, the TURN server requested 10 mins, but timed out at 6, so I set it at 4 below 50%)
But if it works now as intended, even better.

@Sheikah45
Copy link
Member Author

Understood. I appreciate the background as to why

@Sheikah45 Sheikah45 merged commit 4c79734 into master Nov 10, 2023
1 check passed
@Sheikah45 Sheikah45 deleted the maintenance/update-ice4j branch November 10, 2023 22:11
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