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

Remove dependency on transport.Net from client #276

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

stv0g
Copy link
Member

@stv0g stv0g commented Nov 15, 2022

This PR removes the vnet from the TURN client and passes TURN/STUN server addresses as net.Addr's directly.
I also prefer passing a resolved net.Addr instead of doing the address resolution inside NewClient.

I currently do not see a reason why the TURN client needs to have a knowledge about the vnet as we already pass a net.Conn to the client.

This makes the API a lot simpler and more similiar to the pion/stun module which does not use the vnet at all.
If possible, I would even try to remove any reference to vnet from pion/turn completely.

This PR will change the API and hence should bump the module to v2

What do you think?

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Patch coverage: 70.73% and project coverage change: -0.03 ⚠️

Comparison is base (42f695a) 67.94% compared to head (80d5c25) 67.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   67.94%   67.91%   -0.03%     
==========================================
  Files          39       39              
  Lines        2505     2475      -30     
==========================================
- Hits         1702     1681      -21     
+ Misses        668      662       -6     
+ Partials      135      132       -3     
Flag Coverage Δ
go 67.91% <70.73%> (-0.03%) ⬇️
wasm 44.79% <17.64%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
internal/allocation/allocation.go 82.85% <0.00%> (ø)
internal/allocation/channel_bind.go 60.00% <ø> (ø)
internal/allocation/permission.go 53.84% <ø> (ø)
internal/client/binding.go 93.82% <ø> (ø)
internal/client/permission.go 100.00% <ø> (ø)
internal/proto/chandata.go 100.00% <ø> (ø)
internal/proto/connection_id.go 0.00% <0.00%> (ø)
internal/server/turn.go 59.25% <ø> (ø)
internal/server/util.go 50.54% <ø> (ø)
lt_cred.go 55.88% <ø> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stv0g stv0g requested a review from at-wat November 15, 2022 19:22
Copy link
Member

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

This PR removes the vnet from the TURN client and passes TURN/STUN server addresses as net.Addr's directly.
I also prefer passing a resolved net.Addr instead of doing the address resolution inside NewClient.

Sounds reasonable to me, but I haven't worked on this repository.
It would be better to ask @Sean-Der or @enobufs for review.

client.go Outdated Show resolved Hide resolved
@stv0g stv0g force-pushed the client-no-net branch 3 times, most recently from 35e7ee2 to 4a6d0ec Compare January 12, 2023 20:42
@stv0g
Copy link
Member Author

stv0g commented Jan 12, 2023

@Sean-Der @enobufs

It would be fantastic if you could have a look at the PR 🤩

I am finally trying to adapt pion/turn to pion/transport/v2.
As this PR and #271 are both breaking the API, we should merge them both for pion/turn/v2.

This was referenced Jan 12, 2023
@stv0g stv0g marked this pull request as draft February 2, 2023 19:46
@stv0g stv0g marked this pull request as ready for review February 22, 2023 12:43
@stv0g stv0g requested a review from at-wat February 22, 2023 12:43
@enobufs
Copy link
Member

enobufs commented Feb 23, 2023

This PR will change the API and hence should bump the module to v2

It appears the latest tag is v2.1.0. I believe this is going to be v3.

Copy link
Member

@enobufs enobufs left a comment

Choose a reason for hiding this comment

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

It looks good to me.

Copy link
Member

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

LGTM


Leaving a comment out-of-the-scope for this PR:

It's little hard to do re-review since the PR contains cosmetic changes and also rebased since the last view.
(No problem on this PR as we only use rebase-merge mode and the commit of cosmetic changes is properly separated from the main commit)

GitHub web UI shows witch files are changed since the last review if PR branch is not rebased.
It might be better to discuss to switch to the squash-merge mode to allow having intermediate commits in PRs.

@stv0g
Copy link
Member Author

stv0g commented Feb 24, 2023

It appears the latest tag is v2.1.0. I believe this is going to be v3.

Thanks @enobufs for the review. Maybe lets for a few more API breaking changes before we make this a v3.
I couldnt identify any other upcoming breaking changes by looking at the open issues & PRs..

There is a slightly related PR in pion/stun#134, which introduces a new stun.DialURI() function.
It can be used in conjunction with pion/turn.NewClient(). I am still pondering whether we also want to have a function in pion/turn to create a client directly by an TURN URI.

Do you have an opinion on that?

It's little hard to do re-review since the PR contains cosmetic changes and also rebased since the last view.
(No problem on this PR as we only use rebase-merge mode and the commit of cosmetic changes is properly separated from the main commit)

Sorry for this. I will do such changes in a separated PR in the future.

Remove dependency on transport.Net from client
Fix types and comment capitalization
@rg0now
Copy link
Contributor

rg0now commented Feb 24, 2023

I am still pondering whether we also want to have a function in pion/turn to create a client directly by an TURN URI.

I think this would be a nice addition that would make the life of people that just want a quick TURN client connection a lot more easier. Currently it takes some 70 lines of code in the UDP example to create the socket, set up the Client, and then calling client.Listen and client.Allocate, it would ne a big win to cut this down to 1-5 lines for simple use cases that do not require a fully fledged client.

I'm wondering though how to pass the realm and the TURN credentials in to the Dial method, maybe we would need a simple Dialer that can hold these for the Dial?

@stv0g stv0g merged commit b674ab6 into pion:master Apr 12, 2023
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.

4 participants