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

Panics in browser environment #511

Closed
boxdot opened this issue Feb 3, 2021 · 13 comments · Fixed by #537
Closed

Panics in browser environment #511

boxdot opened this issue Feb 3, 2021 · 13 comments · Fixed by #537

Comments

@boxdot
Copy link
Contributor

boxdot commented Feb 3, 2021

Panic happens due to usage of Instant::now() which is not implemented in the stdlib for the wasm32-unknown-unknown target.

A solution would be to use a more general wasm-timer crate, which re-exports std::time::Instant for native targets and uses web_sys to implement it for wasm32.

Here is a patch which I am using for a while in production: boxdot@a14c89c.

Please let me know if it is acceptable, then I will create a PR.

@carllerche
Copy link
Collaborator

I will let @seanmonstar weigh-in more definitively. I am not sure how h2 is used in wasm. My gut would be to only depend on wasm-timer when on the wasm platform (a cfg dependency).

@seanmonstar
Copy link
Member

Is there a reason that std::time cannot be made to work on wasm? That would seem like the best option, as it wouldn't need special support everywhere else. But maybe there's a good reason I haven't thought of.

@CryZe
Copy link

CryZe commented Feb 3, 2021

WASM (and especially wasm32-unknown-unknown) doesn't mean you are running inside a browser. WASM is a portable bytecode. Unfortunately they designed wasm-bindgen without introducing a browser specific target. (And honestly wasm32-unknown-unknown should've never had std in the first place).

@boxdot
Copy link
Contributor Author

boxdot commented Feb 6, 2021

Yes, the unspecified environment in wasm32-unknown-unknown is unfortunate.

Just to give you a motivation for my patch of h2: We are using the tonic/hyper/h2 stack to run streaming client-side gRPC in browser for several months already. This is not implemented by any browser atm, but Rust makes it possible. This works really really well.

More technical details: hyper allows to specify an executor, which makes it possible to replace tokio by wasm-bindgen-futures. tonic supports custom connectors and just needed a small patch to replace the runtime in tower/buffer. The only place which is not wasm32/browser compatible is h2. Http/2 connections are tunneled through a websocket.

How about following the getrandom approach? It uses the js feature to enable support in browser based on JS engine. At least, this is then explicit about the fact that the time::now is based on browser's JS runtime.

@boxdot
Copy link
Contributor Author

boxdot commented Apr 15, 2021

@seanmonstar: Any update on this? It would be great to come up with a solution that makes everybody happy. :) Then we could move forward with tonic support of wasm. Please also take a look at the getrandom approach: https://docs.rs/getrandom/0.2.2/getrandom/#webassembly-support

@seanmonstar
Copy link
Member

I've poked the #hyper channel to see if there's more thoughts. I don't want to be in the way of doing cool stuff in WASM. Though at the same time, I still struggle with this: if std::time doesn't want to assume there's a browser, then should h2 really do make that assumption instead?

I see you mention adding a cargo feature, that's one possibility... I'm not sure if it's purest usage of features, since ideally features would work anywhere. But I dunno...

@seanmonstar
Copy link
Member

Isn't there a WASM working group for Rust? Do they have any guidance on how to support browsers without assuming the target? Or any advice at all?

@boxdot
Copy link
Contributor Author

boxdot commented Apr 16, 2021

@seanmonstar Thank you for coming back to this. I asked the wasm wg. They recommend to use https://crates.io/crates/instant. I will prepare a PR if it is fine with you.

@seanmonstar
Copy link
Member

Thanks for checking in with them! Btw, how did you find where to do that? I tried looking but wasn't sure...

As for the suggestion of the instant crate, it does look nicer. But I worry about another part: it creates a fairly loose but public dependency on the exact version of the instant crate. For instance, if a user is trying to use tonic in the browser, and so adds instant = { version = "0.1", features = ["stdweb"] } to their crate, and all is working. Then, we (in h2) decide to upgrade to a new version of instant, or change to a different internal crate, then suddenly that would break the user. And I'd like to stabilize hyper in the coming months, so adding a "public" unstable dependency makes me nervous about that too.

But! I've thought of two other possibilities. The first one is the easiest:

  1. If you configure h2 to set the max_concurrent_reset_streams to 0, I think it shouldn't need to keep track of the instant at all. And if it does, we can fix that with an if check before grabbing that instant.
  2. We could add a builder option to set a clock source function, that defaults to Instant::now(), but could be customized in a WASM environment to check performance.now().

@boxdot
Copy link
Contributor Author

boxdot commented Apr 23, 2021

I understand the problem with implicit public dependency on instant now. Upgrading it is basically a breaking change.

About the two alternatives: I checked the source of tonic. It uses hyper and does not directly use h2. I think this is the usual situation for most clients of hyper. Both changes of h2 would require a change of hyper to expose the config option to the h2 (client) builder. Do you think this is acceptable?

@seanmonstar
Copy link
Member

That's likely fine!

@boxdot
Copy link
Contributor Author

boxdot commented May 3, 2021

Here are the patches of h2 and hyper for the first approach:

@seanmonstar, if you think this the right direction, I'll create the corresponding PRs. It still feels to be slightly too implicit. However, I tried out this approach with tonic, and it works quite well.

About the second approach: this, unfortunately, does not seem to work, since there is no Instant type that works for all platforms. It is not clear to me what should be the signature of the clock source setter. I guess this is the reason for the instant crate to exist in the first place.

@seanmonstar
Copy link
Member

Yea, it's a little implicit... But at the same time, it's not really doing anything crazy extra. It's just exposing a configuration that might otherwise be useful. 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants