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

Add a new interface transport.Net #204

Merged
merged 6 commits into from
Nov 19, 2022
Merged

Add a new interface transport.Net #204

merged 6 commits into from
Nov 19, 2022

Conversation

stv0g
Copy link
Member

@stv0g stv0g commented Sep 1, 2022

Description

With two initial implementations

  • stdnet.Net the functions from the standard libraries net package
  • vnet.Net the virtual network for testing

This PR closes #34 and clearly separates the virtual network 'vnet.Net' from the code
which is using in production. Furthermore this change allows users to provide their own
transport.Net implementation for reasons like the one described in #34.

This PR changes some of the core API for this package, why we might consider bumping it to v2.
I have also prepared associated PRs for pion/ice as well as pion/turn which will be linked here shortly.

Reference issue

Closes #34
Closes #207
Closes #168

@stv0g stv0g changed the title Add a new interface 'net.Net' Add a new interface net.Net Sep 1, 2022
@stv0g
Copy link
Member Author

stv0g commented Sep 2, 2022

@Sean-Der Can you unblock to CI for me? I think you already approved the idea of the PR in #168.

@stv0g
Copy link
Member Author

stv0g commented Sep 10, 2022

@Sean-Der I fixed the CI warnings. Can you re-trigger the pipeline please?

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 84.63% // Head: 81.70% // Decreases project coverage by -2.92% ⚠️

Coverage data is based on head (86b987f) compared to base (a284a0e).
Patch coverage: 61.83% of modified lines in pull request are covered.

❗ Current head 86b987f differs from pull request most recent head 0b684cf. Consider uploading reports for the commit 0b684cf to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   84.63%   81.70%   -2.93%     
==========================================
  Files          31       32       +1     
  Lines        2695     2750      +55     
==========================================
- Hits         2281     2247      -34     
- Misses        311      394      +83     
- Partials      103      109       +6     
Flag Coverage Δ
go 81.51% <61.83%> (-2.86%) ⬇️
wasm 77.20% <59.31%> (-0.28%) ⬇️

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

Impacted Files Coverage Δ
replaydetector/replaydetector.go 90.32% <ø> (ø)
vnet/resolver.go 82.00% <0.00%> (ø)
vnet/tbf.go 92.85% <ø> (ø)
vnet/udpproxy.go 78.57% <20.00%> (-2.17%) ⬇️
vnet/conn.go 62.79% <47.36%> (-10.19%) ⬇️
test/util.go 79.31% <50.00%> (ø)
vnet/net.go 64.45% <57.28%> (-11.32%) ⬇️
stdnet/net.go 61.19% <61.19%> (ø)
vnet/router.go 81.40% <92.30%> (+0.05%) ⬆️
net.go 100.00% <100.00%> (ø)
... and 5 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.

@stv0g
Copy link
Member Author

stv0g commented Oct 9, 2022

Okay, yet another try please.
Only a single job (the commit message linter) failed.

I think this is ready to be merged @Sean-Der?

@stv0g
Copy link
Member Author

stv0g commented Oct 16, 2022

Hi @enobufs, @Sean-Der,

is there anything I can do to help getting this PR getting merged?

Do we need more maintainers for Pion?

@stv0g stv0g mentioned this pull request Oct 17, 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.

Changes look good to me. (just one question)
Before merging this, I'd like to discuss with you how we want to roll out this change. I personally feel we should bump a major version. We'd also need to think about how to update pion/ice and pion/webrtc. Any thoughts?

vnet/net.go Show resolved Hide resolved
@enobufs
Copy link
Member

enobufs commented Oct 31, 2022

@Sean-Der @at-wat Is there anything I should be aware when we bump a major version of this package?

@stv0g
Copy link
Member Author

stv0g commented Oct 31, 2022

@enobufs I already drafted a bunch of dependent PRs for other Pion packages which will use the new Net interface.

I will update those PRs once the v2 is released.

@at-wat
Copy link
Member

at-wat commented Nov 1, 2022

@enobufs If there are other pending breaking changes, it would be nice to include them in v2.0.0.
We can tag v2.0.0-rc.0 after this PR if there might be other changes, and tag v2.0.0 after settled.

net/interface.go Outdated
@@ -1,28 +1,20 @@
package vnet
package net
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to avoid using package name of stdlib.

Copy link
Member

@enobufs enobufs Nov 1, 2022

Choose a reason for hiding this comment

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

I think @at-wat has a good point.
I have thought about this for a while... I'd like to avoid vnet for the interface package. How about transport? I know it's 9 letters instead of 3, but it's "mostly" about transport layer and sounds right to me. It is also sort of promoting @stv0g 's "Net" interface to be the part of the primary package of this repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

@enobufs I agree :) I will implement this change

@stv0g stv0g changed the title Add a new interface net.Net Add a new interface transport.Net Nov 14, 2022
@stv0g stv0g force-pushed the net-interface branch 5 times, most recently from 1648b2b to 65d97ff Compare November 15, 2022 11:46
@stv0g stv0g requested review from enobufs and at-wat November 15, 2022 11:48
@stv0g
Copy link
Member Author

stv0g commented Nov 15, 2022

Hi @enobufs,

I did some more work and moved the Net interface to the main package of the transport module.
I also started to extend the interface with support for TCP connections.
My goal is to extend the interface as much as we need to cover all uses of the standard Go net package throughout the various Pion packages where it makes sense:

  • Dial, DialUDP, DialTCP
  • Listen, ListenPacket, ListenTCP, ListenUDP
  • ResolveIPAddr, ResolveTCPAddr, ResolveUDPAddr
  • Interfaces, InterfaceByName, InterfaceByIndex
  • more?

@stv0g
Copy link
Member Author

stv0g commented Nov 16, 2022

More changes:

  • Harmonized return types of {stdnet,vnet}.NewNet()
  • Fix typos
  • Fix UT assertions
  • Add Net.InterfaceBy{Name,Index}()
  • Make CI pass

@enobufs
Copy link
Member

enobufs commented Nov 19, 2022

I did not realize earlier that this package had not even reached v1 yet. I am thinking about this work to be the release candidate as v1. (if necessary, we could create v0 (maintenance branch) if necessary (probably not). @at-wat @Sean-Der let me know if you have any thoughts/concerns.

Once this landed on master, I will tag it as v1.0.0-beta to facilitate further work on pion/ice.

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.

Great work! Are you able to rebase to fix the conflict? Other than that it looks good.

@at-wat
Copy link
Member

at-wat commented Nov 19, 2022

Once this landed on master, I will tag it as v1.0.0-beta to facilitate further work on pion/ice.

v0 may have breaking change without bumping major version according to the semantic versioning.
(I don't have strong opinion about this)

@enobufs
Copy link
Member

enobufs commented Nov 19, 2022

v0 may have breaking change without bumping major version according to the semantic versioning.

True. We could just bump it to v0.14.0 for now, but this package should have v1 soon.

With initial implementations by 'stdnet.Net' and
'vnet.Net'. This PR closes pion#34 and clearly
separates the virtual network 'vnet.Net' from the code
which is using in production. Furthermore this change
allows users to provide their own 'net.Net' implementation
for reasons like the one described in pion#34.
Only implemented for stdnet.Net so far.
Fix some errors found by golangci-lint
Add more interface getters to Net
Return error in vnet.NewNet()
Also abort tests early on fatal errors
@stv0g stv0g merged commit 30bbb28 into pion:master Nov 19, 2022
@stv0g
Copy link
Member Author

stv0g commented Nov 19, 2022

Thanks @enobufs and @at-wat :)

I've rebased and merged the changes. I expect that we might need some more minor patches to make it work with all the other Pion modules. After that we could think about tagging v1 in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redefining vnet vnet: Introduce interface for vnet.Net vnet: NewNet() should return an error
3 participants