fix(time): stop wall-clock default from panicking on wasm32 - #704
Merged
Conversation
ChronoTimeProvider called chrono::Utc::now() with no cfg-gate; on wasm32 (no wasmbind backend) that falls through to SystemTime::now(), which panics on the first timestamp. The monotonic clock already cfg-splits with a wasm fallback; the wall clock was left un-gated. cfg-gate ChronoTimeProvider to non-wasm32 and add UnsetWasmTimeProvider (returns epoch + warns once) for wasm32, selected via a cfg-split default_time_provider(), mirroring default_monotonic_provider(). No new deps; embedders register a real provider via set_time_provider().
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe time provider selection is now cfg-gated: native builds default to a chrono-backed ChronoTimeProvider; wasm builds default to UnsetWasmTimeProvider which returns epoch millis (0) and emits a one-time warning when first used without a registered provider. ChangesArchitecture-Specific Time Provider Selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5addf0f6aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Benchmark Results67 unchanged benchmark(s)
|
An early now_millis() on wasm32 stored UnsetWasmTimeProvider in the OnceLock, so a later set_time_provider() returned Err and the wall clock (and the monotonic clock that derives from it) stayed at epoch forever. Use the epoch fallback transiently and only read a provider once one is set, so set_time_provider() always wins.
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.
Problem
wacore::time's default wall-clock provider,ChronoTimeProvider, callschrono::Utc::now()with no cfg-gate. Onwasm32-unknown-unknownchrono has no backend (we don't pullwasmbind), soUtc::now()falls through toSystemTime::now(), which panics ("time not implemented on this platform"). Nothing in the repo callsset_time_provider, so a wasm embedder that forgets to register one panics on the first timestamp (any stanza, app-state mutation, or log line).This is an asymmetry with the monotonic clock, which already cfg-splits: native
StdMonotonicProvider, wasmWallDerivedMonotonicProvider. Only the wall clock was left un-gated, and the wasm CI is build-only so it never caught the runtime panic.Fix
Mirror the monotonic clock's split:
ChronoTimeProvideris now#[cfg(not(target_arch = "wasm32"))].UnsetWasmTimeProvider, which returns epoch (0) and warns once. The wall clock has no internal source on wasm32 (and 0 is safe fornow_utc()/now_secs_u64()), so embedders register a real provider viaset_time_provider(e.g. backed byDate.now()).now_millis()selects via a cfg-splitdefault_time_provider(), exactly likenow_nanos()/default_monotonic_provider().This keeps
wacoredependency-free on wasm32 (nojs-sys/wasm-bindgen). The alternative, enabling chrono'swasmbindfeature only for the wasm32 target, would makeUtc::now()work out of the box but pull inwasm-bindgen; happy to switch if you'd prefer zero-config over leanness.Tests
Native
native_default_time_provider_returns_real_timeguards that the native default still yields a real (post-2020) timestamp after the cfg-split. The wasm32 default is panic-free by construction (it no longer callschrono::Utc::now()); confirmed by buildingwhatsapp-rust --target wasm32-unknown-unknownlocally (the wasm runtime panic itself can't be exercised in CI, which is build-only).Breaking
None. Native behavior is unchanged. On wasm32 the default goes from "panic on first timestamp" to "epoch + warning until a provider is set", which is strictly safer.