HalfEdgeMesh cannot be shared between threads safely. #80

DamenH · 2023-01-29T20:36:35Z

This makes it difficult to use the HalfEdgeMesh in a Bevy component, for example.

Adding the sync feature flag to blackjack should fix this.

The text was updated successfully, but these errors were encountered:

setzer22 · 2023-02-23T12:20:20Z

Hi! Thanks for reporting 😄 As we discussed on discord, I'm aware of the issue and unfortunately, there are no easy workaround that can be implemented externally in a completely safe way. A HalfEdgeMesh is both !Sync and !Send, the latter occurs because there is an API which the lua bindings use to get a reference to a "shared" channel. That shared channel is backed by an Rc, so obtaining a shared channel and then sending the HalfEdgeMesh to another thread while that shared channel is still held alive somewhere would cause trouble. This is why common workarounds like wrapping HalfEdgeMesh in a Mutex don't work, because mutex is only Send if its contents are.

To summarize the task: I'd rather this be a "don't pay for what you don't use" thing, so unfortunately we can't simply go through the source and replace all the RefCells and Rcs with their thread-safe counterparts. That would make the library less performant when you don't actually need to share HalfEdgeMesh between threads (which is in most cases).

The best way to approach this, would be to create wrappers for an "interior mutability" type and a "shared ownership" type. The implementation would simply delegate to RefCell / RwLock and Rc / Arc using conditional compilation.

The wrapper types are necessary because the different types don't offer a shared API, so we need the wrapper type to know when to call lock() or borrow() depending on a compile-time feature flag and offer a clean unified API to the rest of the codebase. The wrapper type it would be easier to swap the internal implementation with something else (e.g. replace the RwLock with an AtomicRefCell).

This is not difficult work, but it can be a bit tedious. I'm currenlty focused on the GUI rewrite, so I can't tackle this right now. PRs are welcome as we discussed 😄

But in the meantime, maybe I can suggest a different architecture, similar to what I did for the godot-rust plugin. Instead of storing HalfEdgeMesh as bevy components, create a thin component type containing an id, something that is cheaply Copy-able like a slotmap key. Then, the meshes are actually managed inside a bevy non-send resource. This is similar to how bevy already handles assets, where the asset key is stored inside components, and to access the actual asset data, your system needs to depend on an additional resource. This definitely makes the architecture a bit more complex, but can be a good way to work around the issue.

DamenH mentioned this issue Feb 26, 2023

Feature/sync #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

HalfEdgeMesh cannot be shared between threads safely. #80

HalfEdgeMesh cannot be shared between threads safely. #80

Uh oh!

Uh oh!

HalfEdgeMesh cannot be shared between threads safely. #80

HalfEdgeMesh cannot be shared between threads safely. #80

Comments

Uh oh!