Global improvement for wasm_thread

[erwanvivien]

First commit adds

• A way to prefix all created workers, usefull when you want to isolate some of them easily
• A way to force all created workers to target one script. Allows to spawn worker in workers where the script_path.js doesn't work

Second commit adds

• Scoped thread API, pretty much everything is pulled from std::thread, so it shouldn't be that bad, it's actually a borrow checker thing, not a real implementation

Third commit adds

• Unsafe narrowing where needed

[chemicstry]

Thanks for the PR!

2nd and 3rd commits look good.

However, I'm not sure about the global vars for setting URL and name prefix. Why not just use Builder to provide any custom options? It is less convenient to pass it each time, but having a global state in a library is a bad idea. For example if one of your dependencies also uses wasm-thread, then using set_wasm_bindgen_shim_script_path will break it.

[erwanvivien]

My use case for the 1st commit is that I'm plug-in wasm into a lib, and changing each thread builder was a pain, but I'm ok removing it

[erwanvivien]

But anyway, the js script doesn't work for workers in workers :/

[erwanvivien]

I think you're right that mutable variable in a lib is bad

[chemicstry]

I haven't used wasm/webworkers in a while so I'm trying to remember all the quirks. Now that I think about it, there isn't a usecase where you would use different wasm bindgen scripts (correct me) in the same binary without doing some really weird things. So it might make sense to setup configuration only once on startup and then have all the dependencies use the same configuration.

I'll need some time to think about it.

[erwanvivien]

And to be honest, most worker.js are identical

[chemicstry]

Okay, so instead of having separate set_wasm_bindgen_shim_script_path and set_worker_prefix functions, I think it would be much cleaner to be able to set default Builder options. We could manually implement Default for Builder where it takes options from a global static. Then also add a method fn Builder::set_as_default(&self), which sets the global static.

So at the start of main, you could do:

wasm_thread::Builder::default()
  .wasm_bindgen_shim_url("somewhere/script.js")
  .prefix("wasm_thread_")
  .set_as_default();

Prefixing should work as such. Builder has two options: prefix: String (default "") and name: Option<String> (default None). The final worker name is just a join of prefix and name, however if name is None, then it is generated.

Btw, there is no need to introduce new dependencies for global variables, you could just do: static DEFAULT_BUILDER: Mutex<Option<Builder>> = Mutex::new(None);

Let me know what you think. If you agree, then update your first commit with these changes. Or revert it so I can merge the other two and I will implement the rest.

[erwanvivien]

I'll make a draft replacing my first commit & let me know if that ok for you :)

[erwanvivien]

@chemicstry I think it's close to what you want :)

[erwanvivien]

I repushed commit n°1 because it included a trash include

[chemicstry]

Thanks!

A tip for future: create separate PRs so that you don't have to rebase commits on each change, everything can be squashed during merge. It's also easier to review.

[erwanvivien]

Yeah, my bad! Thanks

[erwanvivien]

Could you ping me when you'll release it on crates.io ? :) Upgrading my Cargo.toml would be nice :)

[chemicstry]

Yes, I want to do some refactoring and cleanup before releasing though. Will ping you once done.

[erwanvivien]

Very good

[chemicstry]

@erwanvivien It turned out to be quite a large refactor, so could you check if #14 did not break anything for you?

Also, I noticed that neither this nor any older version of the library works on newest google chrome. The std::thread::sleep() just hangs infinitely. It works on firefox and older chrome versions though. Wondering if this could be a bug in chrome...

[erwanvivien]

@chemicstry I'll look into it! I didn't get notified for the message, I was looking for an update, so sorry for the delayed response

[erwanvivien]

@chemicstry In my company, we also have bugs using Wasm on GChrome 110 & 111, on canary (113) everything works just fine

[erwanvivien]

Looking into PR now, then testing