Skip to content

Don't implement Send or Sync on Wasm#3691

Merged
cwfitzgerald merged 4 commits intogfx-rs:trunkfrom
daxpedda:wasm-send-sync
Jun 29, 2023
Merged

Don't implement Send or Sync on Wasm#3691
cwfitzgerald merged 4 commits intogfx-rs:trunkfrom
daxpedda:wasm-send-sync

Conversation

@daxpedda
Copy link
Copy Markdown
Contributor

@daxpedda daxpedda commented Apr 14, 2023 •
edited
Loading

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
cwfitzgerald
cwfitzgerald requested changes Apr 19, 2023
View reviewed changes
daxpedda
daxpedda commented Apr 19, 2023
View reviewed changes
daxpedda
daxpedda commented Apr 19, 2023
View reviewed changes
@daxpedda daxpedda requested a review from cwfitzgerald April 19, 2023 20:53
@daxpedda
Copy link
Copy Markdown
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.

@teoxoy teoxoy mentioned this pull request May 24, 2023
Closed
cwfitzgerald
cwfitzgerald requested changes Jun 15, 2023
View reviewed changes
Copy link
Copy Markdown
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

@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
Copy Markdown
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?

@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
Copy Markdown
Contributor Author

daxpedda commented Jun 20, 2023 •
edited
Loading

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 daxpedda requested a review from cwfitzgerald June 21, 2023 14:32
@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jun 21, 2023

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
Copy Markdown
Member

cwfitzgerald 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.

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
Copy Markdown
Contributor Author

daxpedda commented Jun 21, 2023 •
edited
Loading

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
Copy Markdown
Member

cwfitzgerald commented Jun 21, 2023

Maybe rename it fragile instead of unsound. Thoughts?

@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jun 21, 2023

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
Copy Markdown
Contributor Author

daxpedda commented Jun 22, 2023

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

daxpedda
daxpedda commented Jun 22, 2023
View reviewed changes
@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jun 22, 2023

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.

cwfitzgerald
cwfitzgerald requested changes Jun 22, 2023
View reviewed changes
@daxpedda daxpedda requested a review from cwfitzgerald June 23, 2023 13:46
@daxpedda
Copy link
Copy Markdown
Contributor Author

daxpedda commented Jun 26, 2023

Rebased.

cwfitzgerald
cwfitzgerald approved these changes Jun 29, 2023
View reviewed changes
Copy link
Copy Markdown
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!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) June 29, 2023 20:19
@cwfitzgerald cwfitzgerald merged commit 88f18ed into gfx-rs:trunk Jun 29, 2023
@teoxoy teoxoy mentioned this pull request Jun 30, 2023
Closed
@expenses
Copy link
Copy Markdown
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
Copy Markdown
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.

@Elabajaba Elabajaba mentioned this pull request Jul 30, 2023
Closed
This was referenced Aug 16, 2023
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@daxpedda @cwfitzgerald @expenses