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

Add support for wasm-bindgen #541

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Add support for wasm-bindgen #541

merged 1 commit into from
Jul 11, 2018

Conversation

alexcrichton
Copy link
Contributor

This commit adds support to implement the rand crate on the
wasm32-unknown-unknown target with the wasm-bindgen-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes #478

@dhardy
Copy link
Member

dhardy commented Jul 6, 2018

Thanks, but I hope someone else can review. (I don't know enough about the target platforms.)

@alexcrichton
Copy link
Contributor Author

@fitzgen want to help take a look at this?

@fitzgen
Copy link
Contributor

fitzgen commented Jul 6, 2018

CI is failing on cargo build --no-default-features:

error: Could not compile `rand`.
warning: build failed, waiting for other jobs to finish...
error: unmatched visibility `pub`
   --> src/lib.rs:897:9
    |
897 |         pub type This;
    |         ^^^
error: aborting due to previous error

Copy link
Contributor

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A few comments below, but I'm also not super familiar with the rand code base.

README.md Outdated
@@ -119,6 +119,10 @@ optional features are available:
- `serde1` enables serialization for some types, via Serde version 1.
- `stdweb` enables support for `OsRng` on `wasm-unknown-unknown` via `stdweb`
combined with `cargo-web`.
- `stdweb` enables support for `OsRng` on `wasm-unknown-unknown` via
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/stdweb/wasm-bindgen/

Also wasm-unknown-unknown should be wasm32-unknown-unknown here and the existing spot above.

use super::OsRngImpl;

#[derive(Clone)]
pub enum OsRng {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this definition exposed publicly, or does it get wrapped? If the former, might want to hide the fact that it is an enum:

pub struct OsRng(OsRngKind);

enum OsRngKind {
    Node(..),
    Browser(..),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah this is wrapped above

@alexcrichton
Copy link
Contributor Author

Updated!

src/rngs/os.rs Outdated
impl OsRngImpl for OsRng {
fn new() -> Result<OsRng, Error> {
if this.window().is_undefined() {
return Ok(OsRng::Node(node_require("crypto")))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the logic inverted here or is it simply assumed that node_require("crypto") will not fail?

I assume that window() being undefined implies we can't use browser-crypto, so I guess this is just missing a check that NodeCrypto is available?

(If it really cannot fail then I don't understand why you return errors below instead of falling back to Node.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it's assumed that require("crypto") won't fail as I figured that node has had that API for quite some time. Is that something that should be checked for?

I figured below it'd be good to test that if we see we're in a browser whether crypto actually works, and falling back to an error if it looks like an older browser or something like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crypto module has been around since Node 0.x so it's safe to assume importing it won't fail.

crypto.randomFillSync was backported to Node 6.x, the oldest version still being maintained, so that's also safe to use blindly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I've updated with a comment about this

src/rngs/os.rs Outdated
}
let crypto: BrowserCrypto = crypto.into();
if crypto.get_random_values_fn().is_undefined() {
let msg = "crypto.getRandomValues is undefined";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "window.crypto.getRandomValues is undefined"?

Either way these aren't very user-friendly messages, though I guess they may not need to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least for browsers window is the ambient environment so I believe that window.crypto is basically the same as a bare crypto

@jsheard
Copy link
Contributor

jsheard commented Jul 7, 2018

Don't forget to update the docs in os.rs (lines 62-64)

/// The bare Wasm target `wasm32-unknown-unknown` tries to call the javascript
/// methods directly, using `stdweb` in combination with `cargo-web`.
/// `wasm-bindgen` is not yet supported.

src/rngs/os.rs Outdated
//
// > A QuotaExceededError DOMException is thrown if the requested
// > length is greater than 65536 bytes.
65536
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't appear to be a limit in Nodes API, should this return 65536 in browsers and usize::MAX on node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dhardy
Copy link
Member

dhardy commented Jul 8, 2018

Ah, thanks for the extra docs; makes that clear 👍

@dhardy
Copy link
Member

dhardy commented Jul 11, 2018

@alexcrichton can you rebase please? I fixed the build errors in the master branch.

Once the CI is green I think this is good to go.

This commit adds support to implement the `rand` crate on the
wasm32-unknown-unknown target with the `wasm-bindgen`-based runtime. This
supports being run both in node.js as well as browsers by delegating
appropriately.

Closes rust-random#478
@alexcrichton
Copy link
Contributor Author

Done!

@dhardy dhardy merged commit da2f7fa into rust-random:master Jul 11, 2018
@dhardy dhardy mentioned this pull request Jul 13, 2018
28 tasks
@David-OConnor
Copy link

David-OConnor commented Jul 29, 2018

It compiles on unknown-unknown for me using the latest nightly, but if I call rand::random from a wasm-bindgen lib, I receive the following error in browser:

wasm-0054f422-3479:2 Uncaught (in promise) RuntimeError: unreachable
    at core::ptr::drop_in_place::h725cef5c01ce1c6b (wasm-function[3479]:1)
    at core::ptr::drop_in_place::h1438bb682828fcde (wasm-function[3478]:30)
    at __rdl_dealloc (wasm-function[3473]:444)
    at std::sys_common::util::abort::hcc613e7d553b3a50 (wasm-function[3471]:122)
    at __rdl_alloc (wasm-function[3472]:95)
    at core::alloc::Layout::from_size_align_unchecked::h7987ae01fa261dac (wasm-function[3126]:206)
    at core::num::_$LT$impl$u20$u32$GT$::to_le::hbbc43fbc8982e72f (wasm-function[3023]:518)
    at core::mem::size_of::hd49dd597db7ba2e6 (wasm-function[3125]:113)
    at alloc::alloc::alloc::h0e20e5218e896b1d (wasm-function[3016]:52)
    at core::num::NonZeroUsize::new_unchecked::h991b4a788ad6ce7a (wasm-function[3018]:246)

Including rand in cargo.toml doesn't trigger this alone, nor does importing it. Using rand::random does, even if the value isn't passed to JS.

@alexcrichton alexcrichton deleted the wbg branch July 30, 2018 17:53
@alexcrichton
Copy link
Contributor Author

@David-OConnor interesting! The "am I in a browser" check I think may be subtly wrong here (as I later discovered) which may be what's at fault here perhaps? In any case that's a pretty bad error message :(

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 this pull request may close these issues.

5 participants