Skip to content
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

Thread tests #16

Open
wants to merge 2 commits into
base: refactor
Choose a base branch
from
Open

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