feat(quinn,quinn-udp): Avoid socket2 and std::net::UdpSocket dependencies in wasm32-unknown-unknown target

[matheus23]

Changes

• Adds a cfg_alias wasm_browser for all(target_family = "wasm", target_os = "unknown") (matching wasm*-*-unknown)
• Makes socket2 a target-dependent dependency
• Code depending on std::net::UdpSocket (e.g. Runtime::wrap_udp_socket or Endpoint::rebind) becomes gated as not(wasm_browser)
• adds web_time as target-dependent dependency in quinn to replace std::time::{Instant, Duration}, which panic in Wasm otherwise.

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.

[matheus23]

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.

[djc]

Okay, I think this all makes sense. Thanks!

[djc]

@Ralith ping?

[mxinden]

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.

[mxinden]

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.
}

[matheus23]

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

[mxinden]

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.

[RealAstolfo]

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

[matheus23]

The newest version that avoids the feature in favor of a cfg alias wasm_browser makes lots of these cfgs simpler, as that cfg is exclusive with unix and windows cfgs.
Please take a look at the latest diff @mxinden I think you'll like it more.

[Ralith]

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.

[matheus23]

Main reason was that I noticed this was similar to e.g. tokio's net feature (something you disable when you build for Wasm similarly).
The cfg-gating also gets a little more complex, e.g:

#[cfg(all(not(all(target_family = "wasm", target_os = "unknown")), unix))]
use std::os::unix::io::AsFd;

But I see that you have cfg-aliases enabled for quinn-udp, so I'll try it with that.

[matheus23]

I've updated the PR to use a cfg alias wasm_browser, which stands for all(target_family = "wasm", target_os = "unknown").
With this, a bunch of cfgs get simpler. E.g. all(not(wasm_browser), unix) simplifies to just unix, since wasm_browser and unix are exclusive.

[matheus23]

The CI failed with:

/usr/bin/ld: final link failed: No space left on device

[gretchenfrage]

The CI failed with:

/usr/bin/ld: final link failed: No space left on device

Indeed... Re-running it, it would be convenient if it's spurious but exciting if it's not.

[gretchenfrage]

@matheus23 I know you saw this on Discord, but could you add this commit to the PR by whatever means you find most convenient to make the CI pass?

[gretchenfrage]

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.

To be honest, I'm not sure this feature gating approach is the best solution. I find it hard to follow, and I worry about our ability to remember why it is that way and what we need to do to keep respecting the needs of the WASM environment in the same way in the future.

If the problem is that a user might need to depend on these items that are used in both quinn and quinn-udp, while not having quinn-udp in the dependency tree (only having quinn in the dependency tree), I see 2 alternatives to feature gating as such:

• As you suggest, factor out some common dependency crate between quinn and quinn-udp. "Cleanest", but I understand the hesitation.
• Create duplicate versions of these items in quinn (e.g. essentially copy/paste quinn_udp::RecvMeta into somewhere in quinn), so that AsyncUdpSocket only references the versions defined in Quinn. Implement From/Into for conversion between them (little bit of boilerplate), and have quinn::runtime::{tokio, async_io} use those conversions.

@Ralith @djc @mxinden thoughts on either of these suggestions?

[djc]

To be honest, I'm not sure this feature gating approach is the best solution. I find it hard to follow, and I worry about our ability to remember why it is that way and what we need to do to keep respecting the needs of the WASM environment in the same way in the future.

IMO the feature gating approach is okay even if it's not the cleanest. Personally, I feel like both alternatives (splitting out yet another smaller crate, or duplicating RecvMeta and Transmit) are probably worse.

Would be good to get another round of @Ralith's review.