Use new pion/transport Net interface #471

stv0g · 2022-09-01T18:08:55Z

See pion/transport#204 for a detailed description

Also closes #509

codecov · 2022-09-01T18:11:47Z

Codecov Report

Base: 78.28% // Head: 77.82% // Decreases project coverage by -0.46% ⚠️

Coverage data is based on head (3f55ead) compared to base (c8cff3a).
Patch coverage: 56.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
- Coverage   78.28%   77.82%   -0.46%     
==========================================
  Files          36       36              
  Lines        4273     4307      +34     
==========================================
+ Hits         3345     3352       +7     
- Misses        722      743      +21     
- Partials      206      212       +6

Flag	Coverage Δ
go	`77.82% <56.00%> (-0.46%)`	⬇️
wasm	`24.79% <0.00%> (-0.20%)`	⬇️

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

Impacted Files	Coverage Δ
agent_config.go	`82.14% <ø> (ø)`
candidate_peer_reflexive.go	`80.64% <ø> (ø)`
candidate_server_reflexive.go	`80.00% <ø> (ø)`
tcp_packet_conn.go	`76.02% <ø> (ø)`
udp_muxed_conn.go	`71.95% <ø> (ø)`
agent.go	`82.01% <43.75%> (-0.41%)`	⬇️
udp_mux.go	`81.25% <50.00%> (-1.03%)`	⬇️
gather.go	`64.57% <51.85%> (-1.41%)`	⬇️
udp_mux_multi.go	`52.94% <52.94%> (-1.78%)`	⬇️
mdns.go	`83.33% <100.00%> (+16.66%)`	⬆️
... and 6 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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

This change adapts pion/ice to use a new interface for most network related operations. The interface was formerly a simple struct vnet.Net which was originally intended to facilicate testing. By replacing it with an interface we have greater flexibility and allow users to hook into the networking stack by providing their own implementation of the interface.

Fix linter warnings

candidate_server_reflexive.go

at-wat · 2023-02-08T03:03:04Z

udp_mux.go

@@ -49,6 +50,7 @@ const maxAddrSize = 512
 type UDPMuxParams struct {
 	Logger  logging.LeveledLogger
 	UDPConn net.PacketConn
+	Net     transport.Net


https://github.com/pion/ice/pull/471/files?diff=unified&w=1#diff-a8e331e8f7ba49ca6692af8fc922967307339ad6fe58eb62549602b8988d9bb3L64-L66

// For unspecified addresses, the correct behavior is to return errListenUnspecified, but // it will break the applications that are already using unspecified UDP connection // with UDPMuxDefault, so print a warn log and create a local address list for mux.

Seems be used only for the fallback to the wrong usage of NewUDPMuxDefault.
If user want to specify transport.New, they should use NewMultiUDPMuxFromPort instead.

It would be better to add some comment to this field.

@stv0g
This is an API change beyond what this PR is aiming for. I think the reason why it uses vnet (or transport.Net) is just to reuse localInterfaces() that requires vnet/transport.Net. In fact, it only requires net.Interfaces, and

vnet.NewNet(nil) // <-- *NetConfig being nil

would return what you can get from the original net.Interfaces, so the code we have here did not intend to use "virtual interface" but to use localInterfaces just for convenience. Therefore, I think we don't need to add transport.Net field to this public struct. (Let's focus on introducing transport.Net for now with minimal impact to users)

I actually tried it without adding the field. However that breaks the tests as the integration will create a UDPMux which attempts to listen on addresses of the real host rather than the virtual interfaces of the vnet.VNet.

This will cause vnet/Net.ListenUDP to fail as it can not bind to a non-existing local address.

For normal (non-test usage) the field will be initialized with nil and hence use the stdnet.Net implementation

@at-wat I added a comment to the new field.

enobufs

I've left one comment. (I will review again when it comes out of the draft state.)

enobufs · 2023-02-08T07:15:14Z

udp_mux.go

@@ -49,6 +50,7 @@ const maxAddrSize = 512
 type UDPMuxParams struct {
 	Logger  logging.LeveledLogger
 	UDPConn net.PacketConn
+	Net     transport.Net


@stv0g
This is an API change beyond what this PR is aiming for. I think the reason why it uses vnet (or transport.Net) is just to reuse localInterfaces() that requires vnet/transport.Net. In fact, it only requires net.Interfaces, and

vnet.NewNet(nil) // <-- *NetConfig being nil

would return what you can get from the original net.Interfaces, so the code we have here did not intend to use "virtual interface" but to use localInterfaces just for convenience. Therefore, I think we don't need to add transport.Net field to this public struct. (Let's focus on introducing transport.Net for now with minimal impact to users)

Remove duplicate package comment

To describe its purpose.

stv0g · 2023-02-08T11:54:05Z

Thanks @enobufs and @at-wat for your reviews.

I will review again when it comes out of the draft state

The PR is now ready for merging when you are happy with it :)

edaniels

Nice! One actual change to make and one suggested.

gather.go

udp_mux_multi.go

Co-authored-by: Eric Daniels <[email protected]>

stv0g · 2023-02-08T16:45:29Z

Thanks @edaniels,

I committed your proposed changed 👍

edaniels

LGTM.

Among other improvements, it fixes a memory leak with srfx conn channels not being closed it also make use of new pion/transport Net interface pion/ice#471

stv0g force-pushed the net-interface branch from 043f826 to 5ab30cc Compare September 2, 2022 06:35

stv0g changed the title ~~WIP: Use new pion/transport Net interface~~ Use new pion/transport Net interface Nov 12, 2022

stv0g marked this pull request as draft November 12, 2022 14:51

stv0g force-pushed the net-interface branch 2 times, most recently from 47468e2 to 1a11255 Compare November 15, 2022 12:36

stv0g mentioned this pull request Nov 20, 2022

Update module github.com/pion/transport to v0.14.0 pion/sctp#248

Closed

1 task

stv0g force-pushed the net-interface branch from 01062c7 to 79e23ba Compare December 2, 2022 23:42

stv0g force-pushed the net-interface branch 3 times, most recently from 9cba962 to d8dba3a Compare January 10, 2023 00:04

stv0g mentioned this pull request Jan 30, 2023

Update module github.com/pion/transport to v2 #509

Closed

1 task

stv0g force-pushed the net-interface branch 2 times, most recently from 7ec3723 to 3fd03ff Compare January 31, 2023 13:34

stv0g marked this pull request as ready for review January 31, 2023 14:32

stv0g force-pushed the net-interface branch from 603ed7d to 0dab6c6 Compare January 31, 2023 14:33

stv0g added 2 commits February 7, 2023 17:24

Fix linter warnings

6743b17

Fix linter warnings

stv0g force-pushed the net-interface branch from 0dab6c6 to 6743b17 Compare February 7, 2023 16:24

stv0g requested review from cnderrauber, ashellunts, at-wat and enobufs February 7, 2023 16:25

stv0g mentioned this pull request Feb 7, 2023

Use new pion/transport Net interface pion/webrtc#2408

Merged

at-wat reviewed Feb 8, 2023

View reviewed changes

enobufs reviewed Feb 8, 2023

View reviewed changes

stv0g added 2 commits February 8, 2023 12:51

Remove duplicate package comment

23ed187

Remove duplicate package comment

Add comment to new Net field in UDPMuxParams

13e936c

To describe its purpose.

edaniels requested changes Feb 8, 2023

View reviewed changes

gather.go Outdated Show resolved Hide resolved

udp_mux_multi.go Outdated Show resolved Hide resolved

Fix comment and error name

3f55ead

Co-authored-by: Eric Daniels <[email protected]>

stv0g force-pushed the net-interface branch from f7a3235 to 3f55ead Compare February 8, 2023 16:44

stv0g requested review from edaniels, at-wat and enobufs February 8, 2023 16:45

edaniels approved these changes Feb 8, 2023

View reviewed changes

stv0g merged commit 228c31f into pion:master Feb 8, 2023

stv0g deleted the net-interface branch February 8, 2023 20:58

mlsmaycon added a commit to netbirdio/netbird that referenced this pull request Feb 24, 2023

Update pion/ICE and its dependencies

3a6294c

Among other improvements, it fixes a memory leak with srfx conn channels not being closed it also make use of new pion/transport Net interface pion/ice#471

mlsmaycon mentioned this pull request Feb 24, 2023

Update pion/ICE and its dependencies netbirdio/netbird#703

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new pion/transport Net interface #471

Use new pion/transport Net interface #471

stv0g commented Sep 1, 2022 •

edited

Loading

codecov bot commented Sep 1, 2022 •

edited

Loading

at-wat Feb 8, 2023

enobufs Feb 8, 2023 •

edited

Loading

stv0g Feb 8, 2023

stv0g Feb 8, 2023

stv0g Feb 8, 2023

enobufs left a comment

enobufs Feb 8, 2023 •

edited

Loading

stv0g commented Feb 8, 2023

edaniels left a comment

stv0g commented Feb 8, 2023

edaniels left a comment

Use new pion/transport Net interface #471

Use new pion/transport Net interface #471

Conversation

stv0g commented Sep 1, 2022 • edited Loading

codecov bot commented Sep 1, 2022 • edited Loading

Codecov Report

at-wat Feb 8, 2023

Choose a reason for hiding this comment

enobufs Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

stv0g Feb 8, 2023

Choose a reason for hiding this comment

stv0g Feb 8, 2023

Choose a reason for hiding this comment

stv0g Feb 8, 2023

Choose a reason for hiding this comment

enobufs left a comment

Choose a reason for hiding this comment

enobufs Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

stv0g commented Feb 8, 2023

edaniels left a comment

Choose a reason for hiding this comment

stv0g commented Feb 8, 2023

edaniels left a comment

Choose a reason for hiding this comment

stv0g commented Sep 1, 2022 •

edited

Loading

codecov bot commented Sep 1, 2022 •

edited

Loading

enobufs Feb 8, 2023 •

edited

Loading

enobufs Feb 8, 2023 •

edited

Loading