Implement section-4.3 from RFC6062 #500
Conversation
|
@JoeTurki @rg0now I am going to try and implement this with lots of small PRs. Not done yet with this, but wanted to get just [0] working, and then get accepting in a subsequent PR. We should also allow the user to provide a customer dialer. I saw this suggested in previous PRs. I will mark this with [0] https://datatracker.ietf.org/doc/html/rfc6062#section-4.3 |
|
@Sean-Der This is sooo dope, this was on my todo list for a while, and I feel bad when people ask for features in pion turn and i direct them to coturn or other implementations instead. |
Sean-Der
force-pushed
the
issue-118
branch
3 times, most recently
from
9a7e56a to
1620a16
Compare
|
I was just wondering how this would change the |
Sean-Der
force-pushed
the
issue-118
branch
6 times, most recently
from
bf4cdbf to
fd2e568
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #500 +/- ##
==========================================
+ Coverage 80.24% 80.59% +0.35%
==========================================
Files 45 46 +1
Lines 2693 2809 +116
==========================================
+ Hits 2161 2264 +103
+ Misses 354 352 -2
- Partials 178 193 +15
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:
|
|
@rg0now I am not sure yet! What do you think of me landing things with zero configuration, and then we add it back when everything is working? |
Sean-Der
force-pushed
the
issue-118
branch
5 times, most recently
from
89b579f to
dc83505
Compare
Relates to #118
|
@JoeTurki @rg0now Ok can I get a review now? It's not future complete, but I think this is a good moment to stop and rip into it :)
At least a few more PRs! |
|
@Sean-Der I'll review and test this :) |
| return 0, err | ||
| } | ||
|
|
||
| conn, err := net.DialTCP("tcp4", nil, &net.TCPAddr{IP: peerAddress.IP, Port: peerAddress.Port}) // nolint: noctx |
There was a problem hiding this comment.
This is the critical point: (1) the Dialer should be an abstract interface and (2) the local address:port must be the transport relay address for the allocation (the TCPAddr we create an allocation.CreateAllocation once we change that to call AllocateConn per the requested transport). The former can be taken care of later, but for the latter we will need to add the SO_REUSEADDR (and maybe also the SO_REUSEPORT? I always forget the difference) otherwise we cannot bind multiple peer connections to the same local addr:port. AFAIK SO_REUSEADDR is supported in all major OSes but SO_REUSEPORT is Linux only
There was a problem hiding this comment.
👍 Ok I think I can get this right.
What do you think of landing this PR and then I can follow up with a small commit just getting that in?
| } | ||
| }() | ||
|
|
||
| if _, err = io.Copy(stunConn.Conn(), tcpConn); err != nil { |
There was a problem hiding this comment.
Shouldn't this one go into the goroutine? I'm confused
There was a problem hiding this comment.
Sorry might be missing the issue :(
We want to block the main Read loop for this connection. This way it doesn't try to read it as a STUN Connection.
io.Copy is uni-directional. I see some examples do the io.Copy in two routines and then use a wait group. That seems like more LoC for same behavior?
+1 for the pragmatic approach It just occurred to me that the same TCP listener can work both for creating UDP relay connections and TCP relay connections depending on the requested-transport. Consider my request for adding a "protocol" field to Otherwise I think the code is pretty nice already! |
|
@rg0now 100% agree with your API. What do you think of landing this, and then I will expose an API so users can adjust the Listener + Dialer. Easier to review/make progress, but if that has downsides happy to merge as one big PR! |
|
Okay i reviewed half of this :) |
Relates to #118