Merge #807

807: Implement Send for Instance r=MarkMcCaskey a=MarkMcCaskey # Review - [x] Create a short description of the the change in the CHANGELOG.md file Resolves #748 WIP ## List of changes ### Commit 1 - `Global`s use Arc instead of RC - Export `Context` and `FuncPointer` manually implement Send - `ImportObject` uses `Arc<Mutex<HashMap<...>>>` Instead of `Rc<RefCell<HashMap<...>>>`; removed `get_namespace` method in favor of continuation style to deal with locking the Mutex - `Func` manually implements `Send` (TODO: this change needs to be checked in depth) ### Commit 2 - `unsafe impl Send for Export {}` (temporary to allow Memory to be not Send) - RefCell -> Mutex in Global and Table - Rc -> Arc in Table - Namespace's `IsExport`s must be `Send` (Done to avoid touching much more of the code (i.e. `trait IsExport: Send` requires a lot -- maybe this is what we should do though) - Make `Host` and `Wasm` `Func`s Send (manual implementation) - Manual implementation for `LocalTable` and `AnyFunc` ### Commit 3 - rm placeholder `unsafe impl Send for Export {}` - Manual implementation for `LocalBacking` and `ImportBacking` (both seemed to be not Send due to direct ownership of mutable pointers in their containers) - ImportObject's state creator Fn trait object is now ` + Send + Sync + 'static` (required because it's in an Arc) - Manually implement Send for `InstanceInner` because it holds a raw pointer, `LocalBacking` and `ImportBacking` are marked Send separately - Memory: All Rc -> Arc (including unshared memory); All RefCell -> Mutex (including unshared memory) - Manual implementation of Send and Sync on `UnsharedMemoryInternal` - Manual implementation of Send and Sync on `SharedMemoryInternal` - Change `runtime-core::unix::memory::Memory.fd` from `Option<Rc<Rawfd>>` to `Option<Arc<Rawfd>>` (not strictly required for this change because Memory has manual implementations of Send and Sync, but Arc seems more correct here and there's no comment justifying the use of Rc) - Manual implementation of Send for `ImportedFunc` Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
wasmerio · Sep 23, 2019 · ae1a864 · ae1a864
2 parents 23c7b20 + 705287c
commit ae1a864
Show file tree

Hide file tree

Showing 11 changed files with 170 additions and 81 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@ Blocks of changes will separated by version increments.
 
 ## **[Unreleased]**
 
+- [#807](https://github.com/wasmerio/wasmer/pull/807) Implement Send for `Instance`, breaking change on `ImportObject`, remove method `get_namespace` replaced with `with_namespace` and `maybe_with_namespace`
 - [#817](https://github.com/wasmerio/wasmer/pull/817) Add document for tracking features across backends and language integrations, [docs/feature_matrix.md]
 - [#823](https://github.com/wasmerio/wasmer/issues/823) Improved Emscripten / WASI integration
 - [#821](https://github.com/wasmerio/wasmer/issues/821) Remove patch version on most deps Cargo manifests.  This gives Wasmer library users more control over which versions of the deps they use.

diff --git a/lib/runtime-core/src/backing.rs b/lib/runtime-core/src/backing.rs
@@ -54,6 +54,9 @@ pub struct LocalBacking {
     pub(crate) internals: Internals,
 }
 
+// Manually implemented because LocalBacking contains raw pointers directly
+unsafe impl Send for LocalBacking {}
+
 impl LocalBacking {
     pub(crate) fn new(
         module: &ModuleInner,
@@ -461,6 +464,9 @@ pub struct ImportBacking {
     pub(crate) vm_globals: BoxedMap<ImportedGlobalIndex, *mut vm::LocalGlobal>,
 }
 
+// manually implemented because ImportBacking contains raw pointers directly
+unsafe impl Send for ImportBacking {}
+
 impl ImportBacking {
     pub fn new(
         module: &ModuleInner,
@@ -536,9 +542,8 @@ fn import_functions(
         let namespace = module.info.namespace_table.get(*namespace_index);
         let name = module.info.name_table.get(*name_index);
 
-        let import = imports
-            .get_namespace(namespace)
-            .and_then(|namespace| namespace.get_export(name));
+        let import =
+            imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name));
         match import {
             Some(Export::Function {
                 func,
@@ -624,9 +629,8 @@ fn import_memories(
         let namespace = module.info.namespace_table.get(*namespace_index);
         let name = module.info.name_table.get(*name_index);
 
-        let memory_import = imports
-            .get_namespace(&namespace)
-            .and_then(|namespace| namespace.get_export(&name));
+        let memory_import =
+            imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name));
         match memory_import {
             Some(Export::Memory(memory)) => {
                 if expected_memory_desc.fits_in_imported(memory.descriptor()) {
@@ -696,9 +700,8 @@ fn import_tables(
         let namespace = module.info.namespace_table.get(*namespace_index);
         let name = module.info.name_table.get(*name_index);
 
-        let table_import = imports
-            .get_namespace(&namespace)
-            .and_then(|namespace| namespace.get_export(&name));
+        let table_import =
+            imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name));
         match table_import {
             Some(Export::Table(mut table)) => {
                 if expected_table_desc.fits_in_imported(table.descriptor()) {
@@ -767,9 +770,8 @@ fn import_globals(
     {
         let namespace = module.info.namespace_table.get(*namespace_index);
         let name = module.info.name_table.get(*name_index);
-        let import = imports
-            .get_namespace(namespace)
-            .and_then(|namespace| namespace.get_export(name));
+        let import =
+            imports.maybe_with_namespace(namespace, |namespace| namespace.get_export(name));
         match import {
             Some(Export::Global(mut global)) => {
                 if global.descriptor() == *imported_global_desc {

diff --git a/lib/runtime-core/src/export.rs b/lib/runtime-core/src/export.rs
@@ -11,6 +11,9 @@ pub enum Context {
     Internal,
 }
 
+// Manually implemented because context contains a raw pointer to Ctx
+unsafe impl Send for Context {}
+
 #[derive(Debug, Clone)]
 pub enum Export {
     Function {
@@ -26,6 +29,9 @@ pub enum Export {
 #[derive(Debug, Clone)]
 pub struct FuncPointer(*const vm::Func);
 
+// Manually implemented because FuncPointer contains a raw pointer to Ctx
+unsafe impl Send for FuncPointer {}
+
 impl FuncPointer {
     /// This needs to be unsafe because there is
     /// no way to check whether the passed function

diff --git a/lib/runtime-core/src/global.rs b/lib/runtime-core/src/global.rs
@@ -4,11 +4,14 @@ use crate::{
     types::{GlobalDescriptor, Type, Value},
     vm,
 };
-use std::{cell::RefCell, fmt, rc::Rc};
+use std::{
+    fmt,
+    sync::{Arc, Mutex},
+};
 
 pub struct Global {
     desc: GlobalDescriptor,
-    storage: Rc<RefCell<vm::LocalGlobal>>,
+    storage: Arc<Mutex<vm::LocalGlobal>>,
 }
 
 impl Global {
@@ -56,7 +59,7 @@ impl Global {
 
         Self {
             desc,
-            storage: Rc::new(RefCell::new(local_global)),
+            storage: Arc::new(Mutex::new(local_global)),
         }
     }
 
@@ -83,7 +86,8 @@ impl Global {
                         Value::V128(x) => x,
                     },
                 };
-                *self.storage.borrow_mut() = local_global;
+                let mut storage = self.storage.lock().unwrap();
+                *storage = local_global;
             } else {
                 panic!("Wrong type for setting this global")
             }
@@ -94,7 +98,8 @@ impl Global {
 
     /// Get the value held by this global.
     pub fn get(&self) -> Value {
-        let data = self.storage.borrow().data;
+        let storage = self.storage.lock().unwrap();
+        let data = storage.data;
 
         match self.desc.ty {
             Type::I32 => Value::I32(data as i32),
@@ -105,8 +110,10 @@ impl Global {
         }
     }
 
+    // TODO: think about this and if this should now be unsafe
     pub(crate) fn vm_local_global(&mut self) -> *mut vm::LocalGlobal {
-        &mut *self.storage.borrow_mut()
+        let mut storage = self.storage.lock().unwrap();
+        &mut *storage
     }
 }
 
@@ -120,7 +127,7 @@ impl Clone for Global {
     fn clone(&self) -> Self {
         Self {
             desc: self.desc,
-            storage: Rc::clone(&self.storage),
+            storage: Arc::clone(&self.storage),
         }
     }
 }

diff --git a/lib/runtime-core/src/import.rs b/lib/runtime-core/src/import.rs
@@ -2,9 +2,9 @@ use crate::export::Export;
 use std::collections::VecDeque;
 use std::collections::{hash_map::Entry, HashMap};
 use std::{
-    cell::{Ref, RefCell},
+    borrow::{Borrow, BorrowMut},
     ffi::c_void,
-    rc::Rc,
+    sync::{Arc, Mutex},
 };
 
 pub trait LikeNamespace {
@@ -45,28 +45,29 @@ impl IsExport for Export {
 /// }
 /// ```
 pub struct ImportObject {
-    map: Rc<RefCell<HashMap<String, Box<dyn LikeNamespace>>>>,
-    pub(crate) state_creator: Option<Rc<dyn Fn() -> (*mut c_void, fn(*mut c_void))>>,
+    map: Arc<Mutex<HashMap<String, Box<dyn LikeNamespace + Send>>>>,
+    pub(crate) state_creator:
+        Option<Arc<dyn Fn() -> (*mut c_void, fn(*mut c_void)) + Send + Sync + 'static>>,
     pub allow_missing_functions: bool,
 }
 
 impl ImportObject {
     /// Create a new `ImportObject`.
     pub fn new() -> Self {
         Self {
-            map: Rc::new(RefCell::new(HashMap::new())),
+            map: Arc::new(Mutex::new(HashMap::new())),
             state_creator: None,
             allow_missing_functions: false,
         }
     }
 
     pub fn new_with_data<F>(state_creator: F) -> Self
     where
-        F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static,
+        F: Fn() -> (*mut c_void, fn(*mut c_void)) + 'static + Send + Sync,
     {
         Self {
-            map: Rc::new(RefCell::new(HashMap::new())),
-            state_creator: Some(Rc::new(state_creator)),
+            map: Arc::new(Mutex::new(HashMap::new())),
+            state_creator: Some(Arc::new(state_creator)),
             allow_missing_functions: false,
         }
     }
@@ -92,9 +93,10 @@ impl ImportObject {
     pub fn register<S, N>(&mut self, name: S, namespace: N) -> Option<Box<dyn LikeNamespace>>
     where
         S: Into<String>,
-        N: LikeNamespace + 'static,
+        N: LikeNamespace + Send + 'static,
     {
-        let mut map = self.map.borrow_mut();
+        let mut guard = self.map.lock().unwrap();
+        let map = guard.borrow_mut();
 
         match map.entry(name.into()) {
             Entry::Vacant(empty) => {
@@ -105,27 +107,49 @@ impl ImportObject {
         }
     }
 
-    pub fn get_namespace(&self, namespace: &str) -> Option<Ref<dyn LikeNamespace + 'static>> {
-        let map_ref = self.map.borrow();
-
+    /// Apply a function on the namespace if it exists
+    /// If your function can fail, consider using `maybe_with_namespace`
+    pub fn with_namespace<Func, InnerRet>(&self, namespace: &str, f: Func) -> Option<InnerRet>
+    where
+        Func: FnOnce(&(dyn LikeNamespace + Send)) -> InnerRet,
+        InnerRet: Sized,
+    {
+        let guard = self.map.lock().unwrap();
+        let map_ref = guard.borrow();
         if map_ref.contains_key(namespace) {
-            Some(Ref::map(map_ref, |map| &*map[namespace]))
+            Some(f(map_ref[namespace].as_ref()))
         } else {
             None
         }
     }
 
+    /// The same as `with_namespace` but takes a function that may fail
+    pub fn maybe_with_namespace<Func, InnerRet>(&self, namespace: &str, f: Func) -> Option<InnerRet>
+    where
+        Func: FnOnce(&(dyn LikeNamespace + Send)) -> Option<InnerRet>,
+        InnerRet: Sized,
+    {
+        let guard = self.map.lock().unwrap();
+        let map_ref = guard.borrow();
+        map_ref
+            .get(namespace)
+            .map(|ns| ns.as_ref())
+            .and_then(|ns| f(ns))
+    }
+
     pub fn clone_ref(&self) -> Self {
         Self {
-            map: Rc::clone(&self.map),
+            map: Arc::clone(&self.map),
             state_creator: self.state_creator.clone(),
             allow_missing_functions: false,
         }
     }
 
     fn get_objects(&self) -> VecDeque<(String, String, Export)> {
         let mut out = VecDeque::new();
-        for (name, ns) in self.map.borrow().iter() {
+        let guard = self.map.lock().unwrap();
+        let map = guard.borrow();
+        for (name, ns) in map.iter() {
             for (id, exp) in ns.get_exports() {
                 out.push_back((name.clone(), id, exp));
             }
@@ -158,7 +182,8 @@ impl IntoIterator for ImportObject {
 
 impl Extend<(String, String, Export)> for ImportObject {
     fn extend<T: IntoIterator<Item = (String, String, Export)>>(&mut self, iter: T) {
-        let mut map = self.map.borrow_mut();
+        let mut guard = self.map.lock().unwrap();
+        let map = guard.borrow_mut();
         for (ns, id, exp) in iter.into_iter() {
             if let Some(like_ns) = map.get_mut(&ns) {
                 like_ns.maybe_insert(&id, exp);
@@ -172,7 +197,7 @@ impl Extend<(String, String, Export)> for ImportObject {
 }
 
 pub struct Namespace {
-    map: HashMap<String, Box<dyn IsExport>>,
+    map: HashMap<String, Box<dyn IsExport + Send>>,
 }
 
 impl Namespace {
@@ -182,10 +207,10 @@ impl Namespace {
         }
     }
 
-    pub fn insert<S, E>(&mut self, name: S, export: E) -> Option<Box<dyn IsExport>>
+    pub fn insert<S, E>(&mut self, name: S, export: E) -> Option<Box<dyn IsExport + Send>>
     where
         S: Into<String>,
-        E: IsExport + 'static,
+        E: IsExport + Send + 'static,
     {
         self.map.insert(name.into(), Box::new(export))
     }
@@ -241,12 +266,14 @@ mod test {
 
         imports1.extend(imports2);
 
-        let cat_ns = imports1.get_namespace("cat").unwrap();
-        assert!(cat_ns.get_export("small").is_some());
+        let small_cat_export =
+            imports1.maybe_with_namespace("cat", |cat_ns| cat_ns.get_export("small"));
+        assert!(small_cat_export.is_some());
 
-        let dog_ns = imports1.get_namespace("dog").unwrap();
-        assert!(dog_ns.get_export("happy").is_some());
-        assert!(dog_ns.get_export("small").is_some());
+        let entries = imports1.maybe_with_namespace("dog", |dog_ns| {
+            Some((dog_ns.get_export("happy")?, dog_ns.get_export("small")?))
+        });
+        assert!(entries.is_some());
     }
 
     #[test]
@@ -264,14 +291,14 @@ mod test {
         };
 
         imports1.extend(imports2);
-        let dog_ns = imports1.get_namespace("dog").unwrap();
+        let happy_dog_entry = imports1
+            .maybe_with_namespace("dog", |dog_ns| dog_ns.get_export("happy"))
+            .unwrap();
 
-        assert!(
-            if let Export::Global(happy_dog_global) = dog_ns.get_export("happy").unwrap() {
-                happy_dog_global.get() == Value::I32(4)
-            } else {
-                false
-            }
-        );
+        assert!(if let Export::Global(happy_dog_global) = happy_dog_entry {
+            happy_dog_global.get() == Value::I32(4)
+        } else {
+            false
+        });
     }
 }
diff --git a/lib/runtime-core/src/instance.rs b/lib/runtime-core/src/instance.rs
@@ -25,6 +25,9 @@ pub(crate) struct InstanceInner {
     pub(crate) vmctx: *mut vm::Ctx,
 }
 
+// manually implemented because InstanceInner contains a raw pointer to Ctx
+unsafe impl Send for InstanceInner {}
+
 impl Drop for InstanceInner {
     fn drop(&mut self) {
         // Drop the vmctx.
@@ -763,3 +766,15 @@ impl<'a> DynFunc<'a> {
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    fn is_send<T: Send>() {}
+
+    #[test]
+    fn test_instance_is_send() {
+        is_send::<Instance>();
+    }
+}