fix: moq-net wasm compatibility#2085
Open
ewindisch wants to merge 5 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ewindisch, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesThis PR adds wasm target support to moq-net. It updates workspace and crate dependencies, introduces a portable clock abstraction for native and wasm targets, and switches runtime timers and instants from Sequence Diagram(s)Not applicable. Related PRs: None identified. Suggested labels: wasm, dependencies, rust Suggested reviewers: moq-net maintainers familiar with async runtime and wasm targets 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/Cargo.toml`:
- Around line 32-43: The new wasm dependencies in Cargo.toml are using inline
versions instead of the workspace-pinned pattern required by the /rs workspace.
Add matching entries for wasmtimer, wasm-bindgen-test, and getrandom in the root
[workspace.dependencies], then update the target-specific dependency
declarations here to inherit them with { workspace = true }. Use the existing
target.'cfg(target_family = "wasm")' dependency blocks in moq-net/Cargo.toml as
the location to switch to workspace inheritance.
In `@rs/moq-net/src/model/time.rs`:
- Around line 197-211: The public doc comment on `Time::now()` is stale because
it only describes the native tokio path, while the function now branches on
target family and uses `ClockInstant::now()` on wasm. Update the `now()` doc
comment to reflect both behaviors: mention `tokio::time::Instant::now()` and
`tokio::time::pause` support for non-wasm targets, and note that wasm uses
`ClockInstant::now()` with no tokio pause support.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c3a9e9-1d74-4b73-9700-52c80cb04810
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
Cargo.tomlrs/moq-net/Cargo.tomlrs/moq-net/src/ietf/control.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/model/time.rsrs/moq-net/src/model/track.rsrs/moq-net/src/session.rsrs/moq-net/src/stats.rsrs/moq-net/tests/wasm.rs
Two small changes finish the wasm portability moq-net is already architected for (web-async, kio, web-transport-trait's MaybeSend/MaybeSync): 1. session.rs: cfg-split the SessionInner dyn-erasure wrapper. Native keeps Send + Sync (byte-for-byte the original); on wasm the bounds are dropped so a web_sys::WebTransport-backed Session (!Sync, !Send futures) can satisfy impl<S: web_transport_trait::Session> SessionInner for S. MaybeSend/MaybeSync aren't auto-traits so they can't be dyn bounds — hence the cfg split. 2. Swap real-path tokio::time -> web_async::time (session bandwidth loop, stats ticker, lite/publisher probe, model/track group-cache Instant, ietf/control timeout). tokio::time::Instant::now() panics on wasm (no clock) and its timers need a runtime absent under spawn_local; web_async::time is a 1:1 drop-in (tokio on native, wasmtimer on wasm). Tests keep tokio::time (the pause/advance mock clock isn't re-exported). Requires web-async 0.1.4. model/time.rs Timescale generation stays on tokio::time (producer-side; a subscriber only decodes Timescales). Native build + 336 tests pass; wasm verified via a downstream browser subscriber.
The producer path panicked on wasm: TIME_ANCHOR used std::time::Instant::now()
+ SystemTime::now() (both panic on wasm32 — no clock), Timescale::now() used
tokio::time::Instant, and From<tokio::Instant> called into_std().
cfg-split the anchor's clock types: native uses std::time (unchanged, still
tokio-stubbable via tokio::time::pause in tests); wasm uses wasmtimer::std::
{Instant, SystemTime} (backed by performance.now()/Date.now()). Timescale::now()
picks tokio (native) vs the wasmtimer monotonic clock (wasm); the tokio->Instant
From impl is now native-only. Native behavior is byte-for-byte unchanged.
With this, a browser can PRODUCE moq streams (not just subscribe): Timescale
stamps groups from a real wall clock. Native build + tests unchanged (378 pass,
0 fail).
…lock Adds tests/wasm.rs (cfg wasm32) exercising the model layer on wasm32: - timescale_now_is_sane_and_monotonic: Time::now() returns a real post-2020, monotonic wall-clock time — proves the wasmtimer producer clock works (this panicked before the producer fix). - produce_consume_frame_roundtrip: in-process Broadcast produce -> consume of a frame, covering both directions without a WebTransport session. Run via cargo (NOT wasm-pack, which builds the crate's native-only lib unit tests — they use tokio::spawn and don't compile on wasm): CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner RUSTFLAGS='--cfg=web_sys_unstable_apis --cfg=getrandom_backend="wasm_js"' cargo test --test wasm -p moq-net --target wasm32-unknown-unknown Result: 2 passed, 0 failed (Node). getrandom's wasm backend + wasm-bindgen-test are wasm dev-dependencies only (not forced on downstream consumers).
ewindisch
force-pushed
the
ewindisch/wasm-session-and-time
branch
from
80e64ef to
7a2fc05
Compare
- Move wasmtimer/wasm-bindgen-test/getrandom to [workspace.dependencies]
and inherit with { workspace = true }, per the /rs workspace convention.
- Update Time::now() doc comment to describe both the native tokio path
and the wasm wasmtimer-backed clock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes wasm32 support for moq-net by replacing tokio::time with a WASM compatible wrapper (deferring to tokio::time on native).
It also drops Send + Sync bounds on sessions when compiled to the WASM target.