-
Notifications
You must be signed in to change notification settings - Fork 194
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
Fix multithreaded wasm crash (solves #164) #165
Conversation
The CI seems to have failed due to unrelated reason. Can someone rerun? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few minor changes. Also, now that we are depending on js-sys
can you add a TODO where we use Global
and Self_
to replace that functionality with js_sys::global()
src/wasm-bindgen.rs
Outdated
struct BrowserCryptoContext { | ||
crypto: BrowserCrypto, | ||
// A temporary buffer backed by browser memory to avoid multithreaded wasm exception. See issue #164 | ||
buf: Uint8Array, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of having this structure, we could just do:
enum RngSource {
Node(NodeCrypto),
Browser(BrowserCrypto, Uint8Array),
}
and then add the comment about using the temporary buffer when we actually use the temp buffer:
RngSource::Browser(crypto, buf) => {
// getRandomValues does not work with all types of WASM memory, so
// we initially write to browser memory to avoid an exception.
for chunk in dest.chunks_mut(BROWSER_CRYPTO_BUFFER_SIZE) {
...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response, but do you still want me to fix this? Since you force pushed to my branch and already approved it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, fixing this would be preferable (I can also do it when i have time).
src/wasm-bindgen.rs
Outdated
let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32); | ||
|
||
let ctx = BrowserCryptoContext { crypto, buf }; | ||
|
||
return Ok(RngSource::Browser(ctx)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This then becomes:
let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32);
return Ok(RngSource::Browser(crypto, buf));
7e44e40
to
2831ecc
Compare
Ya that should be fine, once we make sure this works, I'll backport it (maybe once we also cleanup the |
Signed-off-by: Joe Richey <[email protected]>
Signed-off-by: Joe Richey <[email protected]>
Solves rust-random#164 for the v0.1 branch Signed-off-by: Joe Richey <[email protected]>
This solves #164 by using a preallocated
Uint8Array
for callingcrypto.getRandomValues
.The
stdweb
implementation seems fine, as it already usesUint8Array
.Should this fix be backported to
0.15
? There are a lot of crates that still use that version.