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

Use web-time for tokio::time::Instant on wasm32-unknown-unknown #6740

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simlay
Copy link
Contributor

@simlay simlay commented Aug 1, 2024

Motivation

tokio::time::Instant and such dependencies compile for wasm32-unknown-unknown however when std::time::Instant::now() is called, it panics. This more or less renders, tokio::time::* useless as they rely on tokio::time::Instant::now() (which then panics in the stdlib):

Solution

I've done my best to try and research the various PRs. This is related to #6178 and tangentially related to #4827.

The web-time crate is referenced in a few issues and is a "Complete drop-in replacement for std::time that works in browsers."

Tokio is a pretty critical crate in the rust ecosystem and I don't take adding a new dependency lightly. When looking the other top level tokio dependencies, there are just two crates of even remotely similar popularity (pin-project-lite and signal-hook) so this dependency is definitely on the lesser-popularity side. The author/maintainer (daxpedda) is a pretty common contributor to winit (where I first recognized the handle) and a bunch of wasm related projects so I think they're pretty responsible (we've not met, we just contribute to the same repos from time to time.)

This is a sibling PR to tokio-rs/tracing#2758.

cc: @daxpedda (hope you don't mind the tag)

There's still the issue that std::thread::sleep is unsupported for wasm which is the next problem. I figure it's good to see the interest level of these changes before getting too far into the weeds.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-time Module: tokio/time T-wasm Topic: Web Assembly labels Aug 1, 2024
@simlay simlay force-pushed the use-web-time-on-wasm32-unknown-unknown branch from 85bd0db to 1359f29 Compare August 1, 2024 20:53
@Darksonn
Copy link
Contributor

Darksonn commented Aug 1, 2024

I was under the impression that std::time and std::thread::sleep works under some wasm targets, but not others. Are you able to clarify this?

@daxpedda
Copy link
Contributor

daxpedda commented Aug 1, 2024

cc: @daxpedda (hope you don't mind the tag)

I appreciate it actually, so I can help out if necessary!

I was under the impression that std::time and std::thread::sleep works under some wasm targets, but not others. Are you able to clarify this?

Both Emscripten and WASI support std::time.
std::thread is supported as well given the right environment and sub target, e.g. wasm32-wasip1-threads.

std::thread::sleep() specifically works on wasm32-unknown-unknown as well if compiled with target-feature= "atomics" (which requires -Zbuild-std).

This all depends of course on the environment. E.g. std::thread::sleep() will fail in browsers under wasm32-unknown-unknown if called on the main thread.

I'm not particularly familiar with Emscripten and WASI, but I'm happy to answer any questions around wasm32-unknown-unknown and browser support.

@Darksonn
Copy link
Contributor

Darksonn commented Aug 1, 2024

The current situation is that Tokio isn't really usable in browser wasm because it will block the UI thread when it goes to sleep. However, I believe it should work from web workers as blocking the worker is okay.

@simlay simlay force-pushed the use-web-time-on-wasm32-unknown-unknown branch 2 times, most recently from f8393e2 to a706a57 Compare August 2, 2024 14:30
@@ -96,6 +96,7 @@ mod instant;
pub use self::instant::Instant;

mod interval;
pub(crate) use instant::InstantInner;
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 don't love this solution but think it might be the least bad? I played with a few different refactors to deal with the Inner of Clock in the cfg_test_util feature block using std::time::Instant. I'm presume is for testing timeouts and such and really didn't want to touch it. I think using the same feature-flagged type alias might be the most maintainable.

Thoughts?

@simlay simlay force-pushed the use-web-time-on-wasm32-unknown-unknown branch from a706a57 to 085ed8e Compare August 2, 2024 14:42
@Darksonn
Copy link
Contributor

Darksonn commented Aug 2, 2024

What cases does this actually improve the situation in? If it only makes a difference in non-web-worker browser-wasm, then I don't think it makes sense.

@Darksonn
Copy link
Contributor

Ping on this?

@C0D3-M4513R
Copy link

C0D3-M4513R commented Aug 14, 2024

What cases does this actually improve the situation in? If it only makes a difference in non-web-worker browser-wasm, then I don't think it makes sense.

I made an app using egui, and I am currently using this via the cargo patch section.
I chose to use tokio for my app, because tokio is my first choice for an async driver on native apps.

My web-app is only a "port" I made of my app.
This pr would actually help me in that situation.

I am unable to demonstrate the app currently because of some web CORS issues (so unrelated to this pr or crate).
I haven't tested my app with browser CORS disabled, but it should work to like 90% (I might have to revisit some persistence things, but again unrelated).

@simlay
Copy link
Contributor Author

simlay commented Aug 14, 2024

What cases does this actually improve the situation in? If it only makes a difference in non-web-worker browser-wasm, then I don't think it makes sense.

Yeah. After authoring this PR and using it more I ran into other issues and wish for more features from the time module. In the time since I authored this PR, I've been looking at how one could use gloo_timers to help with the things in tokio::time::{Interval, Interval, Sleep} but it's the APIs aren't very similar.

My web-app is only a "port" I made of my app.

Yeah, my use case is similar to @C0D3-M4513R's. I have an existing project which I'm trying to support running in the web browser. The existing codebase uses a lot of tokio::time::Instants and a few tokio::time::timeout()s. I've done this with other projects and ended up with similar hurdles.

I agree that it's pretty un-intuitive to be able to do tokio::time::Instant::now() yet unable to use tokio::time::sleep with it. I suppose we could punt/close this PR for now as it doesn't support the other portions of time.

I definitely tried to keep this PR small and straight forward. Would it be of interested to expand the scope to include Timeout, Sleep, and Interval?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-time Module: tokio/time T-wasm Topic: Web Assembly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants