fix(wasm): avoid std::time::Instant::now() and expand testing#625
fix(wasm): avoid std::time::Instant::now() and expand testing#625pmarks wants to merge 4 commits into
Conversation
Currently retry and throttle can crash on WASM due to hitting std::time::Instant::now(), and MultiPart upload relies on tokio::task::JoinSet, which doesn't work on WASM. So do the following: - Use n0-futures, which re-exports tokio types on native, and supplies browser-compatible versions on wasm32. Used to get a replacement for tokio::task::JoinSet, used for MultiPart uploads - Use sleep() from gloo-timers on wasm instead of tokio::time::sleep which relies on std::Instant::now() - To test WASM, we use #[wasm_bindgen_test] instead of #[tokio::test] to mark many existing tests. We can conditionally import the appropriate macro, renaming the macro as `async_test` then decorate the tests with #[async_test]. These tests will now run under the existing WASM CI.
… async tests in wasm.
| pub(crate) async fn sleep(duration: Duration) { | ||
| use send_wrapper::SendWrapper; | ||
| if !duration.is_zero() { | ||
| SendWrapper::new(gloo_timers::future::sleep(duration)).await |
There was a problem hiding this comment.
The inner future is not actually Send because it contains JS objects. SendWrapper makes it Send, with dynamic checking that it is not used on another thread. This works for standard wasm32-unknown-unknown, and will crash 'safely' if used across threads somehow with Web Workers, so we can figure out how to fix that later.
There was a problem hiding this comment.
I wonder if perhaps we should create our own type that is Send whenever the target arch is not wasm32?
I.e.
//! `MaybeSend` and `MaybeSync` traits for cross-platform async code.
//!
//! Reference: <https://github.com/iced-rs/iced/blob/master/futures/src/maybe.rs>
#[cfg(not(target_arch = "wasm32"))]
mod platform {
/// A marker trait that enforces `Send` only on native platforms.
///
pub use core::marker::Send as MaybeSend;
/// A marker trait that enforces `Sync` only on native platforms.
///
pub use core::marker::Sync as MaybeSync;
}
#[cfg(target_arch = "wasm32")]
mod platform {
/// A marker trait that enforces `Send` only on native platforms.
pub trait MaybeSend {}
impl<T> MaybeSend for T {}
/// A marker trait that enforces `Sync` only on native platforms.
pub trait MaybeSync {}
impl<T> MaybeSync for T {}
}
pub use platform::{MaybeSend, MaybeSync};
|
This is a rather high-level question for |
|
Perhaps we should have a |
|
Yeah, I would feel more comfortable w/ an explicit |
|
I am working on organizing a release Wha do we want to do with this PR? |
|
btw theres a new "feature flags" section in the rust docs arrow-rs-object-store/src/lib.rs Lines 522 to 548 in 2e859ee |
5 participants
Proposed fix to #624 - opening for discussion, I'm happy to rework this.
Currently retry and throttle can crash on WASM due to hitting std::time::Instant::now(), and MultiPart upload relies on tokio::task::JoinSet, which doesn't work on WASM.
TODO: figure out a unit test that actually exercises
RetryableRequestin WASM. Existing tests can't run because they need MockServer, which needs TcpListener.Which issue does this PR close?
Closes #624
Rationale for this change
Solidify WASM support by fixing a couple small issues and greatly expanding WASM test coverage.
What changes are included in this PR?
async_testthen decorate the tests with #[async_test]. These tests will now run under the existing WASM CI.Are there any user-facing changes?
The main Error enum will change for the WASM build - it will now expose
n0_future::task::JoinErrorinstead oftokio::task::JoinError- this is a breaking change.