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

TURN client refactor #75

Merged
merged 8 commits into from
Jul 15, 2019
Merged

TURN client refactor #75

merged 8 commits into from
Jul 15, 2019

Conversation

enobufs
Copy link
Member

@enobufs enobufs commented Jul 11, 2019

Resolves #74

Description

Please read the details in #74

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
@Sean-Der
Copy link
Member

Amazing work @enobufs this code was a joy to read, really impressive how quickly you are getting this done.

This will close out a lot of TURN issues, I really appreciate you taking this on!

@enobufs
Copy link
Member Author

enobufs commented Jul 12, 2019

Refresh for allocation and permissions are done. What's left is ChannelBind/ChannelData!

client.go Outdated Show resolved Hide resolved
@Sean-Der
Copy link
Member

Sean-Der commented Jul 12, 2019

Refresh for allocation and permissions are done. What's left is ChannelBind/ChannelData!

PR looks fantastic! Only minor nits.

After this release I am going to take a crack at performance stuff next. Especially with us putting debug/print statements in stuff it worries me. Wish it was compile time a bit :/

Maybe doesn't actually matter though.

Really excited to use this, this is a huge relief. I was really worried we were going to be in a bad spot with issues before.

@enobufs
Copy link
Member Author

enobufs commented Jul 12, 2019

@Sean-Der, @hugoArregui
Besides the details, I guess you are ok with the overall architecture..(?) I have some ideas on how to integrate this new client to pion/ice, but I am not fully confident about my understanding in how pion/ice is implemented. (I should be able to start working on the integration into pion/ice.)

In addition to solving relay-srflx(symmetric NAT) connectivity issue, we can also improve these things with this effort.

Simplified CandiateRelay

With the net.PacketConn interface, we may be able to unify the candidate base. CandidateRelay will be a lot simpler. (I believe, turn.Client will live in CandidateBase)

Demultiplexing

This relates to pion/ice#76. All candidate should share their the same "base". We will need to implement how to demultiplex the inbound packet from the same conn. I have a rough sketch in this doc I believe the current implementation would allow us to achieve that with the new TURN client with Client#HandleInbound().

Connectivity check improvement

I have not filed an issue for this, but I guess you may have already realized. This new client can also improve the way we do connectivity checking (with the built-in transaction/retransmission timers).

@codecov-io
Copy link

codecov-io commented Jul 12, 2019

Codecov Report

Merging #75 into master will increase coverage by 21.74%.
The diff coverage is 68.39%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #75       +/-   ##
===========================================
+ Coverage   41.27%   63.01%   +21.74%     
===========================================
  Files          15       23        +8     
  Lines         831     1617      +786     
===========================================
+ Hits          343     1019      +676     
- Misses        449      466       +17     
- Partials       39      132       +93
Impacted Files Coverage Δ
internal/client/errors.go 0% <0%> (ø)
internal/client/atomic_bool.go 100% <100%> (ø)
internal/client/trylock.go 100% <100%> (ø)
internal/allocation/allocation.go 83.07% <100%> (+0.26%) ⬆️
internal/client/periodic_timer.go 100% <100%> (ø)
stun.go 80% <100%> (+2.22%) ⬆️
internal/client/permission.go 48.71% <48.71%> (ø)
internal/client/conn.go 54.18% <54.18%> (ø)
client.go 69.61% <68.77%> (-12.21%) ⬇️
server.go 58.76% <70.58%> (+12.68%) ⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0df9a6...c6db2e9. Read the comment docs.

@Sean-Der
Copy link
Member

Sean-Der commented Jul 12, 2019

@enobufs this is an A+ for me!

The only thing I would push back on (for now) is sharing sockets between candidate types.

I can build a repro via vnet. But right now since we do STUN/TURN via default route it has a much higher chance of not having timed out requests.

If we do the request on all interfaces we can send out requests that don’t route. This blocks startup time for ~5 seconds. Chromium doesn’t have this issue, they seem to be smart enough to only do it on some interfaces.

So I am for sharing sockets. We just need to be careful about which one outbound STUN/TURN requests happen on I think.


Oh and really nice idea with the transaction/timeout stuff! Nice that we can bring down the duplication. I think this is the right place to do it.

@enobufs
Copy link
Member Author

enobufs commented Jul 12, 2019

Thanks @Sean-Der , good to know you are comfortable with the direction!

Just one thing to make sure we are on the same boat.. (please bear with me)

As you mentioned "default interface", I think you are talking about "multi-homed" cases where we have more than one local IP address or NICs. That is a (separate) issue we will need to sort out. What I'd like to address soon is a different one which is mentioned in pion/ice#74.

Even there is only one local IP (non-loopback) on one NIC, we are allocating three UDP sockets (conns) for each candidate types when gathering candidates.

  1. one for initial host candidate here
  2. one for initial srflx candidate here
  3. one for the relay candidate here

This causing more pings, NAT bindings and unnecessary prflx candidates (wasteful in resources).

Here is what it looks like today:
Screen Shot 2019-07-06 at 4 13 24 PM

  • 192.168.0.1 is Node_L's local IP
  • 27.1.1.1 is the external IP address of Node_L's NAT
  • 1.2.3.4 is the (relay) IP address for TURN server
  • 10.2.0.1 is Node-R's local IP
  • 28.1.1.1 is the external IP address of Node_R's NAT

We should use the same conn allocated for host candidate, srflx and relay candidates.

Implemented allocation & permission refresh
Implemented permissions
Implemented channel binding
Threa-safety improvements
Optimized inbound packet demuxing
Resolves #74
Switched cofg.Software to a string
Fixed errors introduced by the last rebase
Resolves #74
@enobufs
Copy link
Member Author

enobufs commented Jul 14, 2019

I am testing with coturn. It returns 401 error to the refresh request. I'm looking into it.

* Requested Transport attr was add to non-allocate requests incorrectly
* Refresh request was missing username, realm, nonce and integrity
Resolves #74
@enobufs enobufs changed the title [WIP] TURN client refactor TURN client refactor Jul 15, 2019
@enobufs
Copy link
Member Author

enobufs commented Jul 15, 2019

@Sean-Der This is good to go. I am pretty confident because it is working with coturn, too! Let's unblock pion/ice#46!

@enobufs
Copy link
Member Author

enobufs commented Jul 15, 2019

@Sean-Der @hugoArregui
In case that helps, please take a look at the new TURN client example (turn-client) here (in another branch)

I think this demonstrates how the new turn.Client's API looks like and works, with a hope that helps your review!

Actually, this turn-client can be a great tool to quickly diagnose your turn server. (It worked with my coturn server on AWS, over NAT!)

@enobufs
Copy link
Member Author

enobufs commented Jul 15, 2019

Also, this is my implementation note

client.go Outdated Show resolved Hide resolved
@enobufs
Copy link
Member Author

enobufs commented Jul 15, 2019

@Sean-Der any suggestion for the version number? (last ver was v1.2.1)

@enobufs enobufs merged commit cc616bb into master Jul 15, 2019
@enobufs enobufs deleted the refactor/client branch July 15, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

TURN client refactor
3 participants