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

Make js Store Sync #12

Closed
wants to merge 7 commits into from
Closed

Make js Store Sync #12

wants to merge 7 commits into from

Conversation

kayhhh
Copy link

@kayhhh kayhhh commented Mar 7, 2024

Hi, I am trying to use this crate with Bevy, but cannot easily do so without Store being Sync. This is already the case with the wasmtime native build, but for the web build there is a Rc<RefCell<...>> causing issues.

I naively replaced it with Arc<RwLock<...>>, and while I'm not sure whether its correct to do so it let my code compile.

Awesome project by the way great work.

@kayhhh
Copy link
Author

kayhhh commented Mar 7, 2024

Well ended up getting a bit more complicated when I tried using WasiCtx within the Store type but I think thats working now too, by adding Sync to the streams.

@kayhhh kayhhh mentioned this pull request Mar 7, 2024
@kajacx
Copy link
Owner

kajacx commented Mar 10, 2024

I am using this is a Bevy game too and ended up putting my store in an Arc Mutex, which works out too. But matching the Sync-ness of wasmtime's Store is probably a better idea, I will look into the PR next weekend.

@kajacx
Copy link
Owner

kajacx commented Apr 14, 2024

I have made Store Send and Sync in 0.3.2, and also in 0.4.0. Looking at the commits:
@kayhhh
replace LazyModuleMemory with Arc RwLock
eca64d9
@kayhhh
undo LazyModuleMemory Arc change
I totally get it, I made it thread safe too until clippy explained to me that it's useless because ModuleMemory (which contains a JsValue) cannot be sent between threads anyway.

@kajacx kajacx closed this Apr 14, 2024
@kayhhh kayhhh deleted the arc-store branch April 19, 2024 21:36
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.

2 participants