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

Don't implement Send or Sync on Wasm #3691

Merged
merged 4 commits into from
Jun 29, 2023

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Apr 14, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
Fixes #2884.
Addresses #3026.

Description
Currently all types unsafely implement Send and Sync on the Wasm target under the assumption that Wasm doesn't support multi-threading. Which it actually does (on nightly).

This PR removes all these implementations and introduces MaybeSend and MaybeSync supertraits to continue implementing Send and Sync on non-Wasm targets.

Testing
It's not, maybe in the future we could add some Wasm multithreading examples/tests.

@daxpedda daxpedda force-pushed the wasm-send-sync branch 6 times, most recently from 4549ddd to 1e99cd8 Compare April 15, 2023 09:03
@daxpedda daxpedda changed the title [experiment] Don't implement Send or Sync for Wasm types Don't implement Send or Sync on Wasm Apr 15, 2023
@daxpedda daxpedda marked this pull request as ready for review April 15, 2023 09:19
wgpu/src/lib.rs Show resolved Hide resolved
wgpu/src/lib.rs Outdated Show resolved Hide resolved
wgpu/src/lib.rs Show resolved Hide resolved
@daxpedda
Copy link
Contributor Author

daxpedda commented May 9, 2023

@cwfitzgerald happy to rebase when this is ready to be merged, just hit me up.
Don't want to continuously rebase unnecessarily.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a comment queued up for the last 2 months....

The main change this needs is to have a feature to enable Send/Sync on wasm with a scary name like unsound_send_sync_threadless_wasm32 or something

wgpu-types/src/lib.rs Show resolved Hide resolved
@daxpedda daxpedda force-pushed the wasm-send-sync branch 2 times, most recently from 688399a to d50e308 Compare June 20, 2023 12:04
@daxpedda
Copy link
Contributor Author

The main change this needs is to have a feature to enable Send/Sync on wasm with a scary name like unsound_send_sync_threadless_wasm32 or something

It's quite a big change actually, I removed a lot of those Sendable, I might almost rewrite the PR from scratch.
May I ask why this is necessary?

@daxpedda daxpedda force-pushed the wasm-send-sync branch 2 times, most recently from d50e308 to e894565 Compare June 20, 2023 12:20
@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 20, 2023

The main change this needs is to have a feature to enable Send/Sync on wasm with a scary name like unsound_send_sync_threadless_wasm32 or something

It's quite a big change actually, I removed a lot of those Sendable, I might almost rewrite the PR from scratch. May I ask why this is necessary?

Just a suggestion, not less work, instead of a crate feature, we could just let Send and Sync be implemented if not(feature = "atomics").

Another one, we could do that this feature only work on not(feature = "atomics") targets, which makes the most sense actually.

Though the disadvantage is that some libraries might rely on this and wouldn't be compatible with atomics then.

@daxpedda
Copy link
Contributor Author

I added the crate feature. It's all very verbose and I should probably adjust the CI to cover it.
Some documentation of this feature would also be nice.

But before I continue, WDYT @cwfitzgerald?

@daxpedda daxpedda marked this pull request as draft June 21, 2023 14:34
@cwfitzgerald
Copy link
Member

I think this situation is fine, though I'm not sure the feature is technically unsound in this case, as you require atomics to have threads at all.

I'm torn as I think we probably should make it unconditional on atomics, as if someone enables this feature, you get in that same "downstream someone breaks it", but maybe that's a good thing if it would lead to unsoundness.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jun 21, 2023

I think this situation is fine, though I'm not sure the feature is technically unsound in this case, as you require atomics to have threads at all.

True, this isn't unsound then. Do you want me to rename the crate feature?

EDIT: I'm gonna deal with CI and docs tmrw.

@cwfitzgerald
Copy link
Member

Maybe rename it fragile instead of unsound. Thoughts?

@daxpedda
Copy link
Contributor Author

Yeah, that sounds fine!

@daxpedda daxpedda force-pushed the wasm-send-sync branch 2 times, most recently from 10b02e3 to 61decbf Compare June 22, 2023 12:27
@daxpedda daxpedda marked this pull request as ready for review June 22, 2023 12:27
@daxpedda
Copy link
Contributor Author

I renamed the crate feature to fragile-send-sync-non-atomic-wasm.

@daxpedda
Copy link
Contributor Author

I couldn't find a central place where all crate features were documented, would like some guidance on where to document this new crate feature.

@daxpedda
Copy link
Contributor Author

Rebased.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the persistence on this!

@expenses
Copy link
Contributor

expenses commented Jul 3, 2023

Thanks for adding the fragile-send-sync-non-atomic-wasm feature! I just tried this and it broke a ton of stuff as bevy components need to be send + sync: https://docs.rs/bevy_ecs/latest/bevy_ecs/component/trait.Component.html.

@daxpedda
Copy link
Contributor Author

daxpedda commented Jul 3, 2023

I guess bevy would need bevyengine/bevy#6689 to support this properly.

FYI fragile-send-sync-non-atomic-wasm doesn't work with target-feature = "+atomics", which requires nightly anyway.

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.

wgpu calls createBuffer on an undefined from a worker thread on Wasm
3 participants