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

[MM-52268] Add initial support for connecting through TCP candidates #103

Merged
merged 10 commits into from
Jun 21, 2023

Conversation

streamer45
Copy link
Contributor

@streamer45 streamer45 commented May 26, 2023

Summary

PR adds initial support for generating and allowing connection through TCP candidates. This allows the service to be self sufficient even in cases where clients cannot establish UDP connections and can avoid the complexity of deploying dedicated TURN servers.

Initial load-tests didn't show a significant difference in performance. So far I tested TCP only vs UDP only. Would be good to test a more realistic scenario with both candidate types used at the same time.

Some current blockers:

  • More performance tests need to be conducted.
  • There's an issue preventing connectivity when listening on all available local interfaces. Still need to understand if it's a configuration issue on our part or a bug in pion/ice but so far I couldn't figure it out.
    • This was resolved by simplifying the logic in b9665a2
  • There's an issue when shutting down the server, listeners take a long time to be closed causing sporadic timeouts. Needs further investigation.
    • This is the API server graceful shutdown which can take a bit when having long lasting connections (e.g. fetching pprof profiles).
  • Documentation updates.

Ticket Link

https://mattermost.atlassian.net/browse/MM-52268

@streamer45 streamer45 added the Work In Progress Not yet ready for review label May 26, 2023
@streamer45 streamer45 self-assigned this May 26, 2023
@streamer45 streamer45 added the Docs needed Requires documentation label May 30, 2023
@streamer45 streamer45 added 2: Dev Review Requires review by a core committer and removed Work In Progress Not yet ready for review labels May 30, 2023
@streamer45 streamer45 requested a review from cpoile May 30, 2023 11:48
@streamer45 streamer45 added the Do Not Merge Should not be merged until this label is removed label May 30, 2023
@streamer45 streamer45 added this to the v0.11.0 milestone May 30, 2023
cpoile
cpoile previously approved these changes May 31, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Very cool, love it. Questions for my own education.

config/config.sample.toml Show resolved Hide resolved
service/rtc/server.go Outdated Show resolved Hide resolved
service/rtc/utils.go Show resolved Hide resolved
service/rtc/utils.go Show resolved Hide resolved
cpoile
cpoile previously approved these changes Jun 2, 2023
@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 12, 2023
cpoile
cpoile previously approved these changes Jun 16, 2023
@streamer45
Copy link
Contributor Author

@cpoile Apologies but had to push some small docs changes.

@streamer45 streamer45 requested a review from cpoile June 20, 2023 23:17
@cpoile
Copy link
Member

cpoile commented Jun 21, 2023

Maybe we can ask to have this repo exempted from the stale review rule?

@streamer45 streamer45 removed the Do Not Merge Should not be merged until this label is removed label Jun 21, 2023
@streamer45 streamer45 merged commit 9ec9ad2 into master Jun 21, 2023
@streamer45 streamer45 deleted the MM-52268 branch June 21, 2023 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request Docs needed Requires documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants