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

Move network related internal packages to here #57

Closed
4 of 5 tasks
at-wat opened this issue Mar 3, 2020 · 8 comments
Closed
4 of 5 tasks

Move network related internal packages to here #57

at-wat opened this issue Mar 3, 2020 · 8 comments
Milestone

Comments

@at-wat
Copy link
Member

at-wat commented Mar 3, 2020

To make webrtc/internal/mux.Mux, transport/vnet.VNet, and transport/test.Bridge compatible with full net.Conn features (including SetDeadline), it would be better to move dtls/internal/net/deadline to this package. Following packages are also better to be moved to here to make package dependencies clean.

  • dtls/internal/net/deadline (net.Conn compatible deadline timer)
  • dtls/internal/net/connctx (context cancelable Conn)
  • udp
  • dtls/internal/net/dpipe (datagram like pipe)
  • webrtc/internal/mux (packet multiplexer)
@stv0g
Copy link
Member

stv0g commented Nov 15, 2022

This seems to be related to #168 & #204.

@at-wat Can you provide an update which of the points of this issue are missing?
I am willing to work on it :)

@stv0g stv0g added this to the V2 milestone Nov 15, 2022
@stv0g
Copy link
Member

stv0g commented Apr 12, 2023

I have just migrated pion/udp to this repo 🥳

@hasheddan
Copy link
Contributor

@stv0g I noticed that in the pion/udp migration, this commit and the corresponding sync package were not migrated -- was that intentional?

@stv0g
Copy link
Member

stv0g commented Aug 27, 2023

Yes, that was intentional as I believe the removed functionality is also part of the standard libraries sync package.

I was also irritated why the functionality was in this module. It does not seem to be networking related.

@hasheddan
Copy link
Contributor

@stv0g I'm not sure introducing the sync package was the proper fix for pion/udp#74, but it seems as though after migrating pion/udp here we are back to the state in which the data race was observed.

@stv0g
Copy link
Member

stv0g commented Aug 27, 2023

Hi @hasheddan ,

Thanks for the context. I now understand yhe motivation of our custom sync.WaitGroup implementation. I think we can add it back. However I would strongly for adding it into internal/sync. We should also highlight the difference to thevstandard implementation to avoid the same mistake in the future.

@stv0g
Copy link
Member

stv0g commented Aug 27, 2023

@hasheddan I am going to address this in #259

@Sean-Der
Copy link
Member

I wasn't able to move webrtc/internal/mux out because it imports pion/ice.

Closing this for now!

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

No branches or pull requests

4 participants