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

chore: add warning log when exceeding max number of pairs #587

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thomaseizinger
Copy link
Collaborator

We've recently had a bug in Firezone where we generated a lot of unnecessary server-reflexive candidates. This was only caught by chance due to a different bug report from a user.

We do have automated reporting set up whenever parts of our code emit WARN and ERROR logs. Adding a WARN log here allows this automated reporting to kick in if that should happen again.

I think - in general - adding a WARN log here is justified. Exceeding the max number of candidates can technically lead to connectivity problems so informing the user about this via a WARN log seems good.

@algesten
Copy link
Owner

@thomaseizinger looks good.

Won't this give you a false positive for long lived road warriors? Like someone having a laptop over 5G on a train.

@thomaseizinger
Copy link
Collaborator Author

Won't this give you a false positive for long lived road warriors? Like someone having a laptop over 5G on a train.

We reset all connections when we (i.e. the OS) detects roaming. So if your network interfaces change, we discard all candidates and start again. I think this means we should never see an exceeding number of candidates unless there is a bug.

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.

Let's land it, but with a change to ensure it only logs once.

@thomaseizinger
Copy link
Collaborator Author

Let's land it, but with a change to ensure it only logs once.

Once per what? This happens before the loop so only once per removal of overflow candidates.

Do you also want it to not log when we repeatedly add more candidates after that?

@algesten
Copy link
Owner

That's it. Our instances can live for very long. So once per instance of the ice agent.

@thomaseizinger
Copy link
Collaborator Author

thomaseizinger commented Nov 16, 2024

That's it. Our instances can live for very long. So once per instance of the ice agent.

Done. I added a flag that prevents this from logging repeatedly. The flag is reset on ICE restart and when the limit gets changed.

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