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

fix(snownet): don't nominate discarded candidates #4806

Merged
merged 2 commits into from
Apr 29, 2024
Merged

Conversation

thomaseizinger
Copy link
Member

When candidates are being invalidated in str0m, they internally set a discarded flag. This flag was not considered when a candidate that was previously discarded was added again. This can happen when a client roams back to their old network, i.e. going from Ethernet to WiFi and back.

In that case, str0m would still form new pairs using the old discarded candidate which would also eventually get nominated as part of a pair. This would lead to a panic in snownet when we try to find the nominated local candidate as we filter by non-discarded candidates there.

Related: algesten/str0m#508.
Resolves: #4736.

Copy link

vercel bot commented Apr 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
firezone ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 0:17am

@thomaseizinger
Copy link
Member Author

@jamilbk Could you test if #4736 is resolved for you with this patch?

@thomaseizinger thomaseizinger changed the title build(rust): bump str0m build(deps): bump str0m Apr 28, 2024
Copy link

github-actions bot commented Apr 28, 2024

Terraform Cloud Plan Output

Plan: 15 to add, 15 to change, 15 to destroy.

Terraform Cloud Plan

@thomaseizinger thomaseizinger changed the title build(deps): bump str0m fix(snownet): don't nominate discarded candidates Apr 28, 2024
Copy link

github-actions bot commented Apr 28, 2024

Performance Test Results

TCP

Test Name Received/s Sent/s Retransmits
direct-tcp-client2server 242.1 MiB (-1%) 244.7 MiB (-1%) 104 (-71%)
direct-tcp-server2client 251.1 MiB (+3%) 253.0 MiB (+3%) 523 (+77%)
relayed-tcp-client2server 226.2 MiB (-3%) 227.0 MiB (-3%) 231 (-5%)
relayed-tcp-server2client 233.8 MiB (-3%) 234.4 MiB (-3%) 385 (+37%)

UDP

Test Name Total/s Jitter Lost
direct-udp-client2server 500.0 MiB (-0%) 0.03ms (-5%) 40.67% (+5%)
direct-udp-server2client 500.0 MiB (-0%) 0.03ms (-21%) 22.22% (+1%)
relayed-udp-client2server 500.0 MiB (+0%) 0.04ms (+8%) 53.66% (-6%)
relayed-udp-server2client 500.0 MiB (-0%) 0.06ms (+95%) 39.12% (-2%)

@jamilbk jamilbk added this pull request to the merge queue Apr 28, 2024
@jamilbk jamilbk removed this pull request from the merge queue due to a manual request Apr 28, 2024
@jamilbk
Copy link
Member

jamilbk commented Apr 29, 2024

Updating with main to pull in bumped versions so that the Swift workflow will pass.

Copy link
Member

@jamilbk jamilbk left a comment

Choose a reason for hiding this comment

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

Seems to work. It was hard to reproduce the bug before -- It wasn't reliably reproducible by going from Ethernet -> WiFi -> Ethernet.

Did some testing today and haven't been able to cause the panic.

@jamilbk jamilbk added this pull request to the merge queue Apr 29, 2024
Merged via the queue into main with commit e35ff07 Apr 29, 2024
136 checks passed
@jamilbk jamilbk deleted the deps/bump-str0m-0.5.1 branch April 29, 2024 01:09
@jamilbk
Copy link
Member

jamilbk commented Apr 30, 2024

After some more testing it looks like this is causing #4826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connlib intermittently panics when roaming networks
2 participants