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

ListenInfo: Add transport socket flags #1291

Merged
merged 16 commits into from
Jan 3, 2024
Merged

ListenInfo: Add transport socket flags #1291

merged 16 commits into from
Jan 3, 2024

Conversation

ibc
Copy link
Member

@ibc ibc commented Jan 2, 2024

Details

  • Add a new flags option in TransportListenInfo to specify socket flags.
  • Supported flags are:
    /**
     * Disable dual-stack support so only IPv6 is used (only if ip is IPv6).
     */
      ipv6Only?: boolean;
    /**
     * Make different transports bind to the same ip and port (only for UDP).
     * Useful for multicast scenarios with plain transport. Use with caution.
     */
     udpReusePort?: boolean;
  • Those flags correspond to the following ones exposed by libuv:
  • Note that flags.ipv6Only is false by default, which is a behavior change.
  • This PR replaces PR Enables multiple transports to bind to the same port #1266 by offering the same functionality in a generic and extensible way, and without making mediasoup responsible of knowing in detail the supported flags/features in each arch/OS.

TODO

  • Document in website.

@ibc
Copy link
Member Author

ibc commented Jan 2, 2024

Rust hates me:

error[E0204]: the trait `std::marker::Copy` cannot be implemented for this type
  --> rust/src/data_structures.rs:52:17
   |
52 | #[derive(Debug, Copy, Clone, Ord, PartialOrd, Eq, PartialEq, Hash, Deserialize, Serialize)]
   |                 ^^^^
...
67 |     pub flags: Option<Vec<SocketFlag>>,
   |     ---------------------------------- this field does not implement `std::marker::Copy`
   |
note: the `std::marker::Copy` impl for `std::option::Option<std::vec::Vec<data_structures::SocketFlag>>` requires that `std::vec::Vec<data_structures::SocketFlag>: std::marker::Copy`
  --> rust/src/data_structures.rs:67:16
   |
67 |     pub flags: Option<Vec<SocketFlag>>,
   |                ^^^^^^^^^^^^^^^^^^^^^^^
   = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0204`.

@nazar-pc
Copy link
Collaborator

nazar-pc commented Jan 2, 2024

Rust hates me:

It helps you write bug-free code. In this case Copy can no longer be derived because Vec is heap-allocated data structure rather than a bag of bytes. Remove Copy from derive if you have to use a vector there.

worker/fbs/transport.fbs Outdated Show resolved Hide resolved
@ibc
Copy link
Member Author

ibc commented Jan 2, 2024

It helps you write bug-free code. In this case Copy can no longer be derived because Vec is heap-allocated data structure rather than a bag of bytes. Remove Copy from derive if you have to use a vector there.

And what are the consequences of removing Copy from derive in that struct?

@nazar-pc
Copy link
Collaborator

nazar-pc commented Jan 2, 2024

And what are the consequences of removing Copy from derive in that struct?

You'll have to call .clone() on it rather than value being implicitly copied (like it happens with u8 for example). Clone typically means there is some side effect of the cloning, in case of vector the biggest side effect is allocation of memory.

@ibc
Copy link
Member Author

ibc commented Jan 2, 2024

@jmillan I've pushed this: a1405e2

This reverts commit 6e785dc.
@jmillan jmillan self-requested a review January 2, 2024 16:15
@ibc ibc marked this pull request as ready for review January 2, 2024 19:57
@ibc ibc merged commit ac2bbcb into v3 Jan 3, 2024
19 of 20 checks passed
@ibc ibc deleted the add-socket-flags branch January 3, 2024 10:42
@threema-lenny
Copy link

I stumbled upon this. It would have been nice to mention the switch from setting UV_UDP_IPV6ONLY by default to make it optional and user-supplied in the changelog as it was effectively a breaking change in behaviour.

@ibc
Copy link
Member Author

ibc commented Apr 22, 2024

I stumbled upon this. It would have been nice to mention the switch from setting UV_UDP_IPV6ONLY by default to make it optional and user-supplied in the changelog as it was effectively a breaking change in behaviour.

True. I've updated the PR description and added a note in the CHANGELOG file: 2854761

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