-
Notifications
You must be signed in to change notification settings - Fork 357
[RFC] Simplified Client API #526
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (69.76%) 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 #526 +/- ##
==========================================
- Coverage 81.72% 81.16% -0.56%
==========================================
Files 46 47 +1
Lines 3157 3329 +172
==========================================
+ Hits 2580 2702 +122
- Misses 375 416 +41
- Partials 202 211 +9
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:
|
650d7ab to
89d7c2b
Compare
|
I am all for it @rg0now as long as we drop the previous API!
|
Huhh, that's a tough call. I'm afraid this would be a massive rewrite burden on people using pion/turn. Plus the this PR merely calls into the existing API so we cannot remove it right now anyway. And then there's the issue that this API hides some features (like
The
I was hesitating whether to do this but eventually decided to go with Note that you only need to call |
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.
I agree with the permissions API comment, maybe we just need a higher-level API?
Thank you, this is a really cool API.
allocation.go
Outdated
| } | ||
|
|
||
| // NewAllocation creates a TURN allocation that can dial peers and accept incoming connections. | ||
| func NewAllocation(network string, conf *ClientConfig) (*turnAllocation, error) { |
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.
Am i missing something or why we don't return Allocation seems more straightforward to keep the details private?
| if a.closed { | ||
| return nil, ErrAllocationClosed | ||
| } |
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.
Should we lock before checking closed?
allocation.go
Outdated
| case d.acceptCh <- conn: | ||
| // Sent to accept queue. | ||
| default: | ||
| // Accept queue full - conn is still registered, just not queued. |
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.
How do we handle this? do we enqueue it somewhere else? i couldn't find it.
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.
The idea is that when the accept queue is full we just drop the connection. E.g., TCP does some 3way handshake magic in such cases that we cannot reproduce here. However I think we'd better unregister the dropped call first. We should probably also emit a log at some point...
|
|
||
| // Dial establishes a connection to the address through TURN. | ||
| // Network must be "udp" or "tcp". | ||
| func (a *turnAllocation) Dial(network, address string) (net.Conn, error) { |
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.
Maybe if we accept network we should take udp4/udp6/tcp4/tcp6 too.
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.
Very true. We'll get back to this once #528 is merged
89d7c2b to
509a050
Compare
5b6e0bc to
e924e07
Compare
With the upcoming addition of TURN/TCP allocations, the mental complexity of working with the client API is becoming unwieldy. As pion/turn gains adoption in areas beyond WebRTC (VPNs, P2P networks, tunneling), this complexity may become a barrier to entry. This PR proposes a simplified client API that mimics the familiar
DialerandListenerinterfaces, even at the cost of hiding some TURN complexity. For simple use cases this API should be easier to pick up, while the current client API remains available for users that want full control.This is just a humble PR to raise some discussion, definitely not production ready.
The Problem
With TCP allocations, we now have two fundamentally different workflows for connecting to peers:
client.Allocate()-> returnsnet.PacketConn-> useWriteTo/ReadFromclient.AllocateTCP()-> returns*client.TCPAllocation-> callDialTCP()-> returnsnet.ConnLikewise for "accepting" peer connections:
ReadFromon thenet.PacketConnreturned byclient.Allocate()Accepton the*client.TCPAllocationreturned fromclient.AllocateTCP()Plus there's the complexity of managing peer permissions and
client.Listen()that users often forget to call.Proposal
A unified
Allocationinterface that works like a combinedDialer+Listener:Key simplifications:
NewAllocation) for both UDP and TCPDial()handles permission creation internallyAccept()works uniformly across both transport typesnet.Conninstead of protocol-specific typesUsage
Create UDP allocation:
For TCP, just set the network type to "tcp" and pass in a TCP TURN socket.
Dial a peer
Dialcreates permission automatically and returns anet.Connbound to the peer both for UDP and TCP.Accept incoming connections from permitted peers
Note that
Dialautomatically callsCreatePermission, but for listenersCreatePermissionmust be called explicitly.