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

Use new pion/transport Net interface #271

Merged
merged 4 commits into from
Jan 31, 2023
Merged

Conversation

stv0g
Copy link
Member

@stv0g stv0g commented Sep 1, 2022

See pion/transport#204 for a detailed description

@stv0g stv0g changed the title Draft: Use new pion/transport Net interface WIP: Use new pion/transport Net interface Sep 1, 2022
@stv0g stv0g changed the title WIP: Use new pion/transport Net interface Use new pion/transport Net interface Nov 12, 2022
@stv0g stv0g marked this pull request as draft November 12, 2022 15:06
@stv0g stv0g force-pushed the net-interface branch 3 times, most recently from 85e175e to 47fc177 Compare November 19, 2022 13:13
@stv0g stv0g marked this pull request as ready for review November 19, 2022 13:15
@codecov
Copy link

codecov bot commented Nov 19, 2022

Codecov Report

Base: 68.65% // Head: 68.19% // Decreases project coverage by -0.46% ⚠️

Coverage data is based on head (d84cf65) compared to base (06d8ab8).
Patch coverage: 27.27% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #271      +/-   ##
==========================================
- Coverage   68.65%   68.19%   -0.46%     
==========================================
  Files          38       38              
  Lines        2469     2481      +12     
==========================================
- Hits         1695     1692       -3     
- Misses        641      653      +12     
- Partials      133      136       +3     
Flag Coverage Δ
go 68.19% <27.27%> (-0.46%) ⬇️
wasm 45.55% <ø> (ø)

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

Impacted Files Coverage Δ
relay_address_generator_none.go 35.00% <0.00%> (-6.18%) ⬇️
relay_address_generator_range.go 0.00% <0.00%> (ø)
relay_address_generator_static.go 46.42% <40.00%> (-5.58%) ⬇️
client.go 70.95% <57.14%> (-1.33%) ⬇️

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

enobufs
enobufs previously approved these changes Nov 24, 2022
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.

Just one quick question. It all looks good to me.

client.go Show resolved Hide resolved
@stv0g
Copy link
Member Author

stv0g commented Dec 2, 2022

Since, pion/transport is exposed by pion/turn through the ClientConfig struct, we will probably also need to bump the major version of pion/turn

@stv0g
Copy link
Member Author

stv0g commented Dec 3, 2022

@Sean-Der What do you think about my previous comment? Is it justified to bump pion/turn to v3 because of the exposed transport.Net interface which also bumped pion/transport to v2?

@enobufs
Copy link
Member

enobufs commented Dec 5, 2022

@stv0g Sorry.. I couldn't reply to you fast enough.., but one way to address this breaking change is to introduce an interface that is compatible with *vnet.Net (a subset of transport.Net, such as transport.NetBase) first for the current major version (use type assertion inside), then in the subsequent major version change, we could replace it with the full interface, reject old vnet.Net without missing implementations)... See https://go.dev/play/p/CuaJbyBQCcl

@stv0g
Copy link
Member Author

stv0g commented Dec 5, 2022

Hi @enobufs,

I am not so sure if this would work. e.g. Net::DialUDP() now also has a changed signature. It is now returning a UDPConn interface. Not a struct as before.

So the new Net interface was not just extended with additional methods but also has seen some minor changes to align it more with the std net module as well as support different Packet/TCP/UDPConn implementations..

At the same time, I am not so a big fan of dragging along these work-arounds as the NetBase interface.
Is there a reason not to bump turn to the next major version immediately?
It is true that we rarely see a v3/4/5/6 or v123 in current Go modules.
But at the same time, I am wondering why this is not the case...
Upgrading between such more frequent major version bumps should be a relatively minor effort.
And actually something I think should be encouraged rather than having new major versions which rewrite 50% of its API surface.. And on the plus side, we end up with cleaner APIs..

stv0g added 3 commits January 20, 2023 12:52
Use new pion/transport Net interface
Handle errors during address resolution in tests
The new Net interface breaks compatability
in v0. So a new v2 version was created.
@enobufs enobufs dismissed their stale review January 22, 2023 21:17

@stv0g Let me dismiss the review (at least for now) as I am not convinced at this point that this is the right approach.

@stv0g stv0g requested review from backkem and ernado January 23, 2023 07:40
@stv0g
Copy link
Member Author

stv0g commented Jan 23, 2023

Let me dismiss the review (at least for now) as I am not convinced at this point that this is the right approach.

Okay, I will try to find another approach. Meanwhile, I am also curious about @Sean-Der and @backkem or @ernado's input on this :)

@enobufs enobufs self-requested a review January 31, 2023 04:54
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.

Thanks for the patience. Let's us move forward. @Sean-Der a setting engine update incoming!

@stv0g
Copy link
Member Author

stv0g commented Jan 31, 2023

Thanks @enobufs and @Sean-Der for the review and discussion about how we handle migration to pion/transport/v2 on a project-wide level :)

I've created the following wiki page to summarize the changes and migration paths for other Pion modules:

@stv0g stv0g merged commit ea05502 into pion:master Jan 31, 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.

3 participants