chemicstry / wasm_thread

A rust `std::thread` replacement for wasm32 target
Apache License 2.0
123 stars 16 forks source link

Add support for jsFutures and spawn_local #18

Open adamgerhant opened 5 months ago

adamgerhant commented 5 months ago

The current method of executing a future is by blocking the thread with futures::executor::block_on. However, this also blocks the JavaScript event loop, so JavaScript promises will never resolve and awaiting a jsFuture will permanently freeze the thread. In order to await a jsFuture, wasm_bindgen_futures::spawn_local must be used. However, this returns immediately, which currently causes the thread to close before the promise can resolve.

One basic solution is is a simple feature flag keep_worker_alive, which simply prevents the thread from closing. This is implemented in the first commit. Since the thread never closes, spawn_local has enough time to complete any asynchronous code, but keeping threads permanently open is not ideal.

My second commit solves this problem by using a static hashmap to keep track of whether each thread should be open or closed. This hashmap is periodically checked by each thread after running the function. If this value is true, the thread closes and the key-value pair is removed. Since the spawn function initializes the value to true, it will close immediately after running the function, which is how the current implementation works. In the spawn_async, function, the value is initialized to false and will only be set to true once the future is complete. This allows the thread to remain open until the js promise has resolved.

I provide tests for both solutions, and there seems to be no backwards compatibility issues.

chemicstry commented 5 months ago

Thanks for the PR! Sorry for taking some time to review it.

I'm glad that this works for your use case, however, I think the implementation is a bit too complicated. Specifically the worker hashmap and the interval close check are unnecessary in my opinion.

I think a better solution would be to have worker entry point be async by default, then closing worker could be much simpler:

wasm_bindgen(module, memory).catch(...).then(async (wasm) => {
    await wasm.wasm_thread_entry_point(work);
    close();
})

I tried to implement a quick proof of concept, but it requires more rust type and lifetime gymnastics than I expected. So before commiting I wanted to hear your opinion since you are already deeper in this than me. What do you think?

chemicstry commented 5 months ago

There is also a way to close worker from inside the rust code, no need to signal javascript poll loop:

pub fn spawn_async<F>(f: F) -> JoinHandle<()>
where
    F: AsyncClosure,
    F: Send + 'static,
{
    Builder::new()
        .spawn(move || {
            wasm_bindgen_futures::spawn_local(async move {
                f.call_once().await;

                js_sys::eval("self")
                    .unwrap()
                    .dyn_into::<DedicatedWorkerGlobalScope>()
                    .unwrap()
                    .close();
            });
        })
        .expect("failed to spawn thread")
}

The same would have to be implemented for sync spawn.

However, async entry point is still the way to go IMO because you can't implement proper JoinHandle<T> for spawn_async this way.

adamgerhant commented 5 months ago

I do like the idea of an async entry point, but it wouldn’t be a trivial implementation. I believe I was also struggling with trait bounds/lifetimes, since instead of WebWorkerContext boxing a function, it would have to box a future. I didnt spend too much time with this method, but ultimately the goal would be to convert all functions to async (whether spawn or spawn_async), wrap them in a future, send them to the other thread, and await them.

I think a better solution is to modify the provided function to close from Rust when it finishes. I didn’t know this was possible, but if so it should be pretty simple. For a spawn(), a new function that wraps the provided function and then calls .close() could work, and for spawn_async() it would be the same thing except after the .await. I’m not sure how this would affect the JoinHandle though.

adamgerhant commented 4 months ago

I finally added support for JoinHandles for the future passed to spawn_async, and closing the thread from Rust instead of the HashMap method. My new method uses spawn_local to handle the work, where the future is awaited, the returned value is sent through the packet, and the thread is closed. I wrote a new test for this, and everything seems to work, except the keep_worker_alive() test. I'm not sure if the method you suggested for closing a thread from Rust actually works.

Also, if you would like to call in order to code review, hear some other potential solutions I created, or describe another method you had in mind, I think that would be helpful.