Should `wasm_bindgen::__rt::WasmRefCell` be threadsafe?

libs
1

Hello! I currently have a PR up for review in the wasm_bindgen project, and I think it would be helpful to have some more eyes on it. Especially if you're familiar with multithreaded code, atomics, and the Webassembly threading proposal!

I've been experimenting with sharing a Rust object across WebWorkers. For the most part, I followed @alexcrichton's amazing raytracing example. But Alex's example sends a different pointer to each worker, which then owns the pointer. My minimal repro shows how I share the same threadsafe object across many workers.

But my WebWorkers keep throwing errors: "Error: recursive use of an object detected which would lead to unsafe aliasing in rust".

...except my code doesn't have any recursion.

But that's okay, because the error message actually means that I've broken Rust's aliasing rules. Someone must be calling an &mut method.

...except my code doesn't have any &mut methods.

I tracked the error down to wasm_bindgen::__rt::WasmRefCell, a small reimplementation of RefCell that wraps every Rust object we access from JavaScript-land. self.borrow was data racing like nobody's business, often underflowing to usize::max_value()! That's a sentinel value that says someone is writing to the object.

So we have a data race in "safe Rust" (at least in the sense that I didn't use any unsafe blocks, not in the sense that it's okay for me to access __wbg_ptr and toss those pointers to other WebWorkers). This isn't necessarily WasmRefCell's fault. It's not a Sync object, and it wasn't designed to be a Sync object. But making it thread-safe, by replacing borrow: Cell<usize> with an AtomicUsize, solves my problem very neatly, and I don't really have a way to keep using threaded wasm Rust code without it. For now, I'm going to use my PR to keep developing my project.

That said, my proposal does add overhead to, um, every call across the JS/Rust boundary. The compiler is usually able to eliminate the store instructions in WasmRefCell, but that's no longer true when those store instructions are atomic.

What do people think? Should this use of atomics live behind a command-line flag or config option? Should Rust's safety guarantees extend to sharing an object across WebWorkers in a browser, at least when that object is Sync?

2

I wonder how this would work when wasm atomics are not supported. I don't see any cfg to fallback to the old behaviour

1 Like
3

Yes, given that not all browsers support atomics in wasm, this definitely needs to be gated.

4

I think this could be easily hidden behind cfg(target_feature = "atomics") to not let non-atomic users pay the overhead cost here, so I'm less concerned about that.

It would be more interesting to see your use-case for this, I've used multi-threaded Wasm in the browser extensively for a while now and didn't need this. Would you mind sharing more details on what exactly you are trying to achieve? Maybe we can figure out an alternative solution!

1 Like
5

If WasmRefCell is not Sync, then it shouldn't matter that it uses Cell<usize> instead of AtomicUsize. The blame is not at WasmRefCell but at the code that shares the WasmRefCell between threads despite it not implementing Sync.

6

It would be more interesting to see your use-case for this, I've used multi-threaded Wasm in the browser extensively for a while now and didn't need this. Would you mind sharing more details on what exactly you are trying to achieve?

Sure! I'm experimenting with running Monte-Carlo Tree Search to play games at a relatively high level in the browser. The game here is called "Hey, that's my Fish!". Here's my source code and here's a compiled online example.

Note that my compiled example is still single-threaded. I haven't fully ironed out the threads use-case, but more importantly, GitHub Pages doesn't allow me to set COOP/COEP headers. Without changing those headers, you can't use SharedArrayBuffer or WebAssembly threads. I'll set up a multithreaded example on Netlify when I get back from travel.

At any rate, the goal is to run Game::playout() as many times as possible, within some window of thinking time. To increase my playout speed, I found a lock-free MCTS algorithm. Implementing it in Rust was as simple as packing my (f32, u32) pairs into AtomicU64s, and converting my Option<Vec<TreeNode>> into std::sync::OnceLock<Vec<TreeNode>>. From there, removing &mut self from Game.playout() was the proof that my function is thread-safe. I was quite impressed with how easy Rust made things! The main thing in my way now is that the invisible, generated, wasm_bindgen code itself is not thread-safe.

7

Most Javascript objects can't be shared between threads. The only one that can be shared is SharedArrayBuffer. And most can't be sent between threads. And any javascript that can be sent or shared has to be done so explicitly by sending it through a MessagePort. In Rust Send and Sync can't invoke any code when sending/sharing. As such javascript objects can't implement Send and Sync.

8

If WasmRefCell is not Sync , then it shouldn't matter that it uses Cell<usize> instead of AtomicUsize . The blame is not at WasmRefCell but at the code that shares the WasmRefCell between threads despite it not implementing Sync.

This makes sense. But I'm not trying to assign blame, I'm just trying to write an object that can be shared across workers safely. It doesn't look like that's possible with today's wasm_bindgen, which is a shame because people often want to share an object across threads.

It's looking to me like WasmRefCell should stay the way it is, and objects should not be sharable by default. But there should be some way to create a sharable object, like marking your object with #[wasm_bindgen(rw_lock)] or #[wasm_bindgen(shared)], which would put your code behind some new WasmRwLock instead of a WasmRefCell.

Most Javascript objects can't be shared between threads. The only one that can be shared is SharedArrayBuffer. And most can't be sent between threads. And any javascript that can be sent or shared has to be done so explicitly by sending it through a MessagePort. In Rust Send and Sync can't invoke any code when sending/sharing. As such javascript objects can't implement Send and Sync .

...I don't really see how this is relevant, because I'm not trying to send a JS object across threads. I have a Rust object which is Sync, and I can get a pointer to that Rust object, which is therefore Send. So it should be sharable across threads. But when I tried it, I learned the hard way that bindgen is wrapping it in invisible generated Rust code which is not Sync.

1 Like
9

For the record, gating on cfg(target_feature = "atomics") shouldn't be necessary, since AtomicUsize already lowers to effectively just a Cell<usize> when atomics aren't enabled.

10

You could work around this by adding an extra layer of indirection: instead of directly exporting your Rust object to JS, you can export a wrapper around it, which contains an Arc pointing to the original object. Then each thread gets its own wrapper with a copy of that Arc.

Both your example from the PR and htmf happen to work pretty nicely with this, since they're already both wrappers around other types: AtomicI32 and MCTSBot respectively. So you don't actually have to introduce an extra type in this case.

Here's how that would look for your example from the PR:

use std::sync::{
    atomic::{AtomicI32, Ordering},
    Arc,
};

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub struct OpaqueObject {
    val: Arc<AtomicI32>,
}

#[wasm_bindgen]
impl OpaqueObject {
    #[wasm_bindgen(constructor)]
    pub fn new(val: i32) -> Self {
        Self {
            val: Arc::new(AtomicI32::new(val)),
        }
    }

    #[wasm_bindgen]
    pub fn get(&self) -> i32 {
        self.val.load(Ordering::SeqCst)
    }

    #[wasm_bindgen]
    pub fn add(&self, num: i32) -> i32 {
        self.val.fetch_add(num, Ordering::SeqCst)
    }

    #[wasm_bindgen]
    pub fn clone_into_raw(&self) -> *const AtomicI32 {
        Arc::into_raw(Arc::clone(&self.val))
    }

    #[wasm_bindgen]
    pub unsafe fn from_raw(ptr: *const AtomicI32) -> Self {
        Self {
            val: Arc::from_raw(ptr),
        }
    }
}

Then you can use clone_into_raw to get pointers to send to worker threads, and recreate a new OpaqueObject on each worker thread using from_raw.

11

You can take a look at the raytrace-parallel example to see how Rust objects are sent between web workers: sending, receiving. I've used this method successfully for a long time now, using the wasm_bindgen macro is not required in that case at all.

Would that actually cover your use case @jthemphill?

EDIT: Liamolucko's example uses the same method, but instead of Box, Arc was used to be able to share an object instead of just sending it.

12

Would that actually cover your use case @jthemphill?

Yes, using unsafe and Arc::from_raw() or Box::from_raw()works!

1 Like
13

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.