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

refactor(ice): always use latest timing config #568

Conversation

thomaseizinger
Copy link
Collaborator

Currently, the StunTiming configuration gets copied into every new CandidatePair that is created. This means that changes to the timing configuration afterwards are not reflected in existing candidate pairs.

To fix this, we remove the StunTiming field from CandidatePair and instead pass in a reference everywhere where it is needed.

The usecase that I want to support with this is to allow lowering the frequency of STUN bindings in certain situations like idle connections. When a connection is not in use (i.e. there is no other traffic being sent over the UDP socket), detecting a failed connection isn't as time critical because the user isn't using the application. This allows the CPU to go to sleep and preserves battery on e.g. mobile devices.

Copy link
Owner

@algesten algesten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Ideally I'd like to avoid passing this in the constructor. I think we can just make a best effort on the capacity.

src/ice/pair.rs Outdated Show resolved Hide resolved
@lolgesten
Copy link

Great. Can you do a search for STUN_MAX_RETRANS and change those stale doc comments too?

@thomaseizinger
Copy link
Collaborator Author

Great. Can you do a search for STUN_MAX_RETRANS and change those stale doc comments too?

Done!

@algesten algesten merged commit 353923f into algesten:main Sep 27, 2024
22 checks passed
@algesten
Copy link
Owner

Excellent! Thanks!

@thomaseizinger
Copy link
Collaborator Author

Thanks for the prompt turnaround (as always)! :)

@algesten
Copy link
Owner

I aim to please :)

@thomaseizinger
Copy link
Collaborator Author

thomaseizinger commented Sep 27, 2024

In case you are curious, here is the downstream feature: firezone/firezone#6845.

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.

3 participants