Skip to content

Conversation

erwanvivien
Copy link
Contributor

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:

  • On branch refactor the worker spawns & despawns instantly
  • On branch main the worker spawns & does its counting

This example can be built with ./wasm-pack.sh
This example can be run with `npx serve -c serve.json pkg`
@chemicstry
Copy link
Owner

chemicstry commented Apr 9, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

Thanks for both of your insights! Very interesting

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.

3 participants