rustwasm
feat: Support unsafe-single-threaded-traits configuration
Adds an unsafe-single-threaded-traits configuration that auto-implements send and sync for JS types under the single threaded assumption.
This has come up for Cloudflare workers when we want to run Axum or other http router systems that require these traits and we end up doing a whole bunch of custom wrapping code, when really we just require this "hack".
Carefully noted safety and included a compiler error if building with threads.
Thanks for the review, this can work for us - I've updated this to use a configuration instead.
@daxpedda it requires us to write wrappers around all types, and then add send and sync to all those types. See e.g. https://github.com/search?q=repo%3Acloudflare%2Fworkers-rs%20unsafe%20impl%20Sync&type=code.
So this small diff here, avoids every consumer having to deal with this problem with a custom wrapper.
I think if arguing against this quality of life improvement, it would be important to provide a substantial counter argument that outweighs the benefits.
@daxpedda it requires us to write wrappers around all types, and then add send and sync to all those types. See e.g. https://github.com/search?q=repo%3Acloudflare%2Fworkers-rs%20unsafe%20impl%20Sync&type=code.
You could instead use struct SendSyncWrapper<T>(T);. A single wrapper that would work for all of them.
You could instead use struct SendSyncWrapper<T>(T);. A single wrapper that would work for all of them.
What I'm hearing here is alternatives, what I don't hear is counter arguments.
You could instead use struct SendSyncWrapper(T);. A single wrapper that would work for all of them.
What I'm hearing here is alternatives, what I don't hear is counter arguments.
The problem with this feature is that it encourages libraries to rely on it for convenience. Which suddenly makes them incompatible with the atomic target feature even when they might not have been. The fact that JsValue isn't Send + Sync gives users pause to evaluate what they are doing and what kind of repercussions this has. Presenting an easy way out will discourage that.
Which is why the cfg is important, because this makes it much harder to rely on. Though still not ideal.
In general though, I don't know why this specific lever should live in wasm-bindgen in the first place. Not implementing Send + Sync is the correct thing here after all. It seems to me that the issue is local to workers-rs, so it should be addressed there. OP seems to suggest that doing it here is "simpler", but that's where I was suggesting the wrapper, which seemingly addresses the issue. Though I'm not convinced that adding this lever in wasm-bindgen is really worth saving 27 lines of code in the first place.
Maybe I'm missing some context why this should be in wasm-bindgen instead?
The problem with this feature is that it encourages libraries to rely on it for convenience. Which suddenly makes them incompatible with the atomic target feature even when they might not have been. The fact that JsValue isn't Send + Sync gives users pause to evaluate what they are doing and what kind of repercussions this has. Presenting an easy way out will discourage that.
Which is why the cfg is important, because this makes it much harder to rely on. Though still not ideal.
If the feature is not recommended in documentation, clearly marked as unsafe, and requires a compiler flag to activate, then it seems to me that libraries simply won't rely on this feature. Therefore I don't understand how that problem surfaces at all?
It is a feature for platforms like Cloudflare, not libraries. Platforms that know their architecture will support it.
In general though, I don't know why this specific lever should live in wasm-bindgen in the first place.
Any future that captures a JsValue has to deal with this problem, it appears everywhere and requires all Wasm Bindgen types to be wrapped. This is not a minor detail.
If the feature is not recommended in documentation, clearly marked as unsafe, and requires a compiler flag to activate, then it seems to me that libraries simply won't rely on this feature. Therefore I don't understand how that problem surfaces at all?
It is a feature for platforms like Cloudflare, not libraries. Platforms that know their architecture will support it.
I agree.
My argument was just to highlight that this feature isn't free, it comes with all that you just pointed out. It will only ever be an argument because I don't understand exactly the arguments in favor.
In general though, I don't know why this specific lever should live in wasm-bindgen in the first place.
Any future that captures a JsValue has to deal with this problem, it appears everywhere and requires all Wasm Bindgen types to be wrapped. This is not a minor detail.
I don't really particularly understand the issue, can you provide a more detailed example? It seems to me that a simple SendSyncWrapper in the right place should really address the issue exactly where its needed and nowhere else. Which is what it looks like in the workers-rs search link you have sent.
I can safely say that the Rust ecosystem is very often dealing with non-Send/Sync Futures, successfully so. I would like to understand the exact hurdle you are hitting there.
A large portion of Cloudflare Workers users use Axum, which assumes threading support for router futures, with Send and Sync as requirements. JsValue is very common in wasm bindgen apps, providing a strong restriction to usage here. As a result all uses of JsValue require wrapping.
The mismatch of expectations here is that we are bringing a GC type (JsValue) into a context in which thread-safe send is assumed. The model mismatch results in the requirement for hacks. Facilitating the hack with the least fuss, by appreciating that when JS GC objects meet threads in a single threading environment, we shouldn't practically need to jump through compiler hoops, helps by choosing usability over dogma for users. Certainly it's not a best practice by any means, but allowing it for platforms is a quality of life improvement.
This might be a general theme for platforms, so let me know.
Yes, as the platform we control the toolchain for the most part, and use a wasm pack fork to run the build where we can apply these configurations.
Using a generic wrapper seems like very little work to me.
The problem is JsValue can be used in turn by n different Rust types for any n, so while individually each wrapper is little work, n can be arbitrarily large. That is, there isn't a single wrapper around JsValue we can rely on - we don't try to hide wasm bindgen internals from our users. JsValue is a first-class type they can use and need to use with web-sys etc. So the problem extends to all derivative types and type wrappers here. The argument for generics in core is effectively a similar one.
This might be a general theme for platforms, so let me know.
Yes, as the platform we control the toolchain for the most part, and use a wasm pack fork to run the build where we can apply these configurations.
Doesn't that make it less general? It seems to me that this whole feature is very tailored to workers-rs and has no general alignment with e.g. other platforms trying to use wasm-bindgen.
Using a generic wrapper seems like very little work to me.
The problem is JsValue can be used in turn by
ndifferent Rust types for anyn, so while individually each wrapper is little work,ncan be arbitrarily large. That is, there isn't a single wrapper around JsValue we can rely on - we don't try to hide wasm bindgen internals from our users. JsValue is a first-class type they can use and need to use with web-sys etc. So the problem extends to all derivative types and type wrappers here. The argument for generics in core is effectively a similar one.
Okay, so I spent some time looking at workers-rs to understand the issue a bit more. This is definitely a user-facing issue in workers-rs and puts a type overhead on users not just workers-rs. So the problem is not actually addressable by workers-rs which in my eyes makes this a valid feature to add in wasm-bindgen.
However, I should note that while compilation will work because you have your custom toolchain, users will still see errors in rust-analyzer. They would need to add some configuration to make that work. Is that an acceptable tradeoff for workers-rs? I think it could be solved by a simple .cargo/config.toml?
If not, we should discuss alternatives.
However, I should note that while compilation will work because you have your custom toolchain, users will still see errors in rust-analyzer. They would need to add some configuration to make that work. Is that an acceptable tradeoff for workers-rs? I think it could be solved by a simple .cargo/config.toml?
Great point, yes this likely an issue here for us. Expecting extra config could be onerous, especially on backwards compatible upgrade paths. Would be good to discuss further.
Would it make sense to expose entirely different types ie UnsafeSendJsValue under a feature flag thus it becomes extra hard for library developers to rely on this feature while also preserving the DX of not needing a .cargo/config.toml?
@parzivale the problem with that approach is that web-sys and js-sys use JsValue so they'd need to then also have a feature / config that switches over to this new type in turn. Unless we were to fork web-sys and js-sys, which generally isn't advisable either.
@guybedford
This is an alternative solution that solves the Send + Sync problem for JsValue without marking JsValue.
It makes it possible to wrap a JsValue in an Arc and use it with other wrapped JsValues. It also supports async closures and covers some trait operations. It accomplishes this by keeping JsValues on one thread and sending closures from other threads to be executed on it.
I don't know if it is relevant to your issue. I like it because if proper multi-threading existed for WASM, it would allow a worker thread and the main thread to share access safely to a JsValue while still using normal rust code in closures. As an example, a lib like webtorrent could be loaded, then another thread could safely interact with the active torrents managed by the lib through closure sending.
