-
Notifications
You must be signed in to change notification settings - Fork 357
Bind relay connections to the relay address for TCP allocations #514
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
Conversation
|
Damn this is amazing @rg0now thanks for doing this. I would like to keep the package zero dependency. Maybe put reuse functionality in
Let's do that! You have my blanket support to do w/e is the right thing, I am here to approve/merge quick :) |
eabb8e6 to
44788bf
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (64.28%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
==========================================
- Coverage 81.35% 80.90% -0.45%
==========================================
Files 46 46
Lines 2971 3017 +46
==========================================
+ Hits 2417 2441 +24
- Misses 346 364 +18
- Partials 208 212 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
862cb87 to
332d037
Compare
|
See pion/transport#372 and pion/transport#373. Please merge and release pion/transport so that I can update this PR. |
332d037 to
5347a01
Compare
|
Thanks @JoTurk for releasing pion/transport/v4, it did not occur to me how much work this is. Sorry for making you undergo this... Removed external dependency, restored the servers' TCP relay listener to use VNet, and fixed the StaticIP -> StaticIPs transition in pion/transport. We still indirectly depend on pion/transport/v3 (I guess via pion/stun), we may want to wait until new versions are released or merge this as is and do the version updates later. |
npnp, I wanted to do this for months since we started working on pion/SCP and was just waiting for an opportunity :) |
I think we should merge, we already merged breaking changes to this library. |
42216ed to
7547572
Compare
cfe07f8 to
e0c1a8b
Compare
Sean-Der
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful!
e0c1a8b to
8a4fba3
Compare
This is a humble attempt to fix #512
The trick is to use the proper sockopts to create the TCP relay listener and the relay connections (
SO_REUSEADDRand wherever needed,SO_REUSEPORT).Caveats:
RelayGenerator.AllocateConnis back for generating the outbound TCP relay connections, so no more hardcodednet.DialTCPin the server. I think this is for the better: now we can abstract away all network-related functions the server invokes (e.g., to wrap OTel telemetry on top of TCP connections). This sort of fixes the API worries I mentioned in an earlier thread.RelayGenerator.AllocateListenervia pion/transport. The reason is thattransport.Netdoes not (yet?) supportnet.ListenerConfig, which we would need to enforce the reuseport semantics. Note that bothAllocateListenerandAllocateConnmust enforce reuseport, otherwise either of them will fail with "Address already in use". Currently we hard-code thenet.ListenerConfiginAllocateListenerinstead of going throughtransport.Net, which is on the one hand ugly and on the other hand requires adding an even ugliercontext.TODO()inListen. We can easily implement something likeCreateListenerConfigintransport.Net(just like we already haveCreateDialer) and hope someone jumps in to implement TCP in pion/transport, let me know if you want be to send a PR.