chemicstry / wasm_thread

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

Thread tests #16

Open erwanvivien opened 1 year ago

erwanvivien commented 1 year ago

I wanted to understand how the changes in the refactor branch "broke" my code

So I made a minimal reproduce of the bug as an example, also having more example (more end-to-end) might be beneficial

How to test the difference:

- Build the example (wasm_thread_example) with `./wasm-pack.sh`
- Serve the example with `npx serve pkg`
- Go on Chrome
- Open console and see if logs appear (change branch in Cargo.toml (refactor vs main)
- Repeat on different branches

What should you see:

chemicstry commented 1 year ago

Thanks for the minimal bug example, I have identified the bug.

Before refactoring, main thread was identified as the one that was first to spawn a thread, however, I changed that to a check if self is an instance of WorkerGlobalScope. Your example firstly spawns a javascript worker and creates a wasm_thread from there. So the main (old) branch thinks that your worker is the main browser thread, while refactor branch does not.

The need for main thread identification is explained in #5, which implemented sub-worker spawning by sending a spawn request back to the main thread. But this does not seem neccessary now, because worker spawning from inside another worker seem to work just fine. Maybe @DouglasDwyer could explain what exatly the problem was?

DouglasDwyer commented 1 year ago

Hey @chemicstry, good to hear from you. The problem was exactly what you mention - certain browser environments could not spawn web workers from other workers. For example, nested workers were only implemented in WebKit a few months ago. It does look like more browsers have support for this now, so maybe the requirement can be relaxed. But before allowing workers to spawn other workers directly, I would recommend testing against all modern browser vendors :)

erwanvivien commented 1 year ago

Thanks for both of your insights! Very interesting