Feat: Marked SendFuture and SendWrapper as unsafe types#784
Feat: Marked SendFuture and SendWrapper as unsafe types#784parzivale wants to merge 10 commits intocloudflare:mainfrom
Conversation
Moved away from general unsafe impls to better communicate the usage style of SendFutures and how they should be used, removed unneeded Sync + Send impls from the Ai Module (Can instead implement SendWrapper)
|
CI seems to be failing here after rebase. Agreed this is the better user experience, would be happy to land it for the next major release after the current patch release. |
… into remove-unsafe-send
|
@guybedford should be good to merge now |
|
Hey what is missing from this for it to be mergeable? |
|
I posted #861 here which just removes send entirely. Putting this one out there as a stake to question why we need this at all. At the very least I would have expected a failing test. Feedback and thoughts very much welcome. |
|
As you mentioned in #861 this is due to Axum requiring send for it's handlers |
|
Plus a lot of crates require send, I believe it is useful to expose a sensible default for implementing unsafe send |
|
Ok, so just to get my head clear on the framing - Then working through still from fundamentals - why do we need to wrap the whole future? Can't we just make all JS types implement Send? |
|
What if we just had a wasm bindgen flag like |
|
Hmm I mean it could work, you would probably would want both a crate feature and a cli feature as if threading ever came to wasm you would have to deactivate all threading code. Tbh Im nowhere near qualified enough to fully answer this |
|
wasm-bindgen supports threading fine - so we would not allow the flag to exist alongside any use of threading. Let me see if I can hack something together, will post back here shortly. |
|
Yeah it all seems to work, implemented wasm-bindgen/wasm-bindgen#4770 and applied this to #861. Further feedback welcome. |
|
Yep I think yours is the better solution, unless workerd ever plans to move to a threaded execution model this makes everything considerably simpler. |
|
Ill keep this pr open while wasm-bindgen/wasm-bindgen#4770 is open |
guybedford
left a comment
There was a problem hiding this comment.
861 on wasm bindgen has stalled since requiring a cfg for this would make it not work by default in the Rust linter.
Therefore we should land this at least in the mean time. Thanks for your patience I will include this in the release tomorrow.
Just to clarify, this is a non-breaking change correct?
Moved away from general unsafe impls to better communicate the usage style of SendFutures, removed unneeded Sync + Send impls from the Ai Module (Can instead implement SendWrapper).
This will probably constitute an api breakage and should (maybe?) go in the next release if accepted.