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

feat(quinn,quinn-udp): Introduce net feature to allow disabling socket2 and std::net::UdpSocket dependencies #2037

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

matheus23
Copy link
Contributor

@matheus23 matheus23 commented Nov 12, 2024

Changes

  • Adds a default-enabled net feature to quinn-udp and quinn
  • Marks socket2 dependency as optional (only enabled with net feature)
  • Code depending on std::net::UdpSocket (e.g. Runtime::wrap_udp_socket or Endpoint::rebind) becomes gated by net feature
  • Added web_time dependency to quinn-udp and quinn that is only enabled in wasm32-unknown-unknown target

Note: The net feature will be implied by current runtime-* features.

Testing

  • These changes were verified outside this PR with a custom Runtime implementation in the browser and nodejs.
    I'm not contributing this test (yet?) as discussed.
  • The "test (wasm32-unknown-unknown)" github action job was adjusted with a check making sure there's no (import "env" ...) in the quinn .wasm file when built.
    This is a good, even if not quite sufficient effort in making sure the Wasm build doesn't break.

.github/workflows/rust.yml Show resolved Hide resolved
quinn/src/runtime.rs Show resolved Hide resolved
quinn/src/tests.rs Show resolved Hide resolved
quinn-udp/src/lib.rs Show resolved Hide resolved
@matheus23
Copy link
Contributor Author

matheus23 commented Nov 12, 2024

I realized this PR is missing some rationale, so I'm writing this on the top level for better visibility:

Does it still make sense to depend on quinn-udp in the WASM world?

This was one of the first things I was thinking initially, too.

The only definitions that quinn without the net feature needs from quinn-udp now are BATCH_SIZE, RecvMeta, EcnCodepoint and Transmit.

The fundamental problem IMO is that AsyncUdpSocket is tied to quinn and its Runtime, but then AsyncUdpSocket depends on e.g. RecvMeta. quinn-udp and quinn are thus tightly coupled at the moment.

The solution would be to separate out the type definitions of RecvMeta, etc. into another crate that both quinn-udp and quinn depend on. I didn't want to do this as that's quite an invasive refactor.
It is not possible to move RecvMeta etc. to quinn, because then quinn-udp would like to depend on quinn and the other way around.

Are you still using the Runtime trait in your WASM setup?

Yes, there's no way of using quinn without providing a Runtime (e.g. the Endpoint keeps a reference to it).

The alternative would be integrating with quinn-proto only, which basically means reimplementing quinns connection and endpoint drivers, timers, API, etc. That's practically not an option.

@matheus23 matheus23 force-pushed the matheus23/quinn-wasm branch from a934ab4 to a12a85a Compare November 12, 2024 12:59
@matheus23 matheus23 force-pushed the matheus23/quinn-wasm branch from a12a85a to 8b20acd Compare November 12, 2024 13:06
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Okay, I think this all makes sense. Thanks!

@djc djc requested a review from Ralith November 28, 2024 08:16
@matheus23 matheus23 requested a review from mxinden as a code owner December 10, 2024 13:05
@djc
Copy link
Member

djc commented Dec 10, 2024

@Ralith ping?

Copy link
Collaborator

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

I understand the rational above for importing quinn-udp in a WASM context.

That said, for what my opinion is worth here, I can't judge whether it is worth the complexity it introduces. Will defer to the actual Quinn maintainers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of the many cfg(feature = "net"), wouldn't something along the following be simpler?

#[cfg(feature = "net")]
use net::*;

#[cfg(feature = "net")]
mod net {
  // Everything that was previously feature flagged behind net.
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've got a version where I've experimented with that.
It's a much more invasive change, I'm not sure if it's much better?
I can add it to this PR if other people agree: n0-computer@d06b71b

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the commit. I didn't think of the fact that unix.rs and windows.rs would need to be under the net/ module.

At this point I don't have a better idea, nor a preference for the options listed here.

Choose a reason for hiding this comment

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

couldnt windows and unix stuff be stuffed under a "system" "sys" or something like that.

@Ralith
Copy link
Collaborator

Ralith commented Dec 21, 2024

You mentioned that you also considered gating on target instead of a cargo feature. Can you elaborate on why you went with the feature instead? I think exposing a subset API on some targets is less hazardous than a cargo feature, particularly as this is technically a semver break affecting users who have default-features = false and use only a custom runtime.

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.

6 participants