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

Support !Send + !Sync Resources #456

Closed
farnoy opened this issue Sep 7, 2020 · 10 comments · Fixed by #671
Closed

Support !Send + !Sync Resources #456

farnoy opened this issue Sep 7, 2020 · 10 comments · Fixed by #671
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@farnoy
Copy link

farnoy commented Sep 7, 2020

Hi,

I have a type that's neither Send nor Sync and I can't find a way to use this data as part of the Schedule. Resources, System and IntoThreadLocalSystem all have requirements on Send + Sync. Would it be possible to relax this?

@cart cart added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Sep 7, 2020
@cart
Copy link
Member

cart commented Sep 7, 2020

Supporting !Send + !Sync resources is definitely a common enough requirement that we should support it. We've already had folks hit this in a number of places and working around it results in ugly and/or unsafe/incorrect code.

@cart cart changed the title Thread local support is inadequate Support !Send + !Sync Resources Sep 7, 2020
@Waridley
Copy link
Contributor

Waridley commented Sep 8, 2020

Could this also apply to Components? E.g. I have a component that's Send but !Sync, but I only access it in one system, so wrapping it in a Mutex is pointless. However, I don't know if there's any way to enforce in the AppBuilder that I'm only using it in one system when it's !Sync.

@Waridley
Copy link
Contributor

Waridley commented Sep 8, 2020

Also, this will come up for web support, because WinitWindows are !Send + !Sync on WASM.

@kakoeimon
Copy link

Where is the problem with Mutex or RWLock?
About components keep in mind that send and sync are requirements of hecs.

@cart
Copy link
Member

cart commented Sep 12, 2020

Mutex<T> and RWLock<T> both require T to be Send. Additionally, some resource/component types might only work correctly when they have been accessed from the main thread. i think we should try to honor the Send/Sync semantics, otherwise we might encounter undefined behavior.

Adding !Send + !Sync resources should be relatively straightforward. Supporting !Send + !Sync components will be a bit harder, but I'm 100% ok with modifying hecs to support this.

@Waridley
Copy link
Contributor

I suppose wrapping a Send + !Sync component in a Mutex is probably not a big deal if I'm only using it in one system, since the lock should basically always succeed, costing only a couple atomic operations... Still a slight waste, but the bigger issue is probably !Send resources.

@kakoeimon
Copy link

I don't think this is a requirement. I mean you can use send + sync in wasm as long you do not create any threads.
For example I have used hecs with Mutex for wasm.

@smokku
Copy link
Member

smokku commented Sep 17, 2020

Faking it does work in a single threaded wasm runtime:

unsafe impl Send for WinitWindows {}

@chemicstry
Copy link

Supporting threads in wasm is a hard requirement IMO. This can work for the time being, but I'm sure other use cases for !Send will pop up.

I would also caution against bare unsafe impl Send and at least use a wrapper type which checks thread id before accessing like the one Amethyst uses. That way you at least get a panic instead of undefined behavior.

@Waridley
Copy link
Contributor

Yeah, the godot-rust bindings do something similar with LocalCellData which panics if the thread ID is not the same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants