refactor!: Memory API modifications (try_clone/copy, duplicate_in_store)

* Change the try_clone and try_copy methods to return a Result<_, MemoryError> This is a more informative return type than the previous Option<>, and allows handling failure cases better * Remove deprecated duplicate_in_store() method Closes #3810
wasmerio · May 17, 2023 · 0379991 · 0379991
1 parent 1b0dbc2
commit 0379991
Show file tree

Hide file tree

Showing 10 changed files with 40 additions and 82 deletions.
diff --git a/lib/api/src/externals/memory.rs b/lib/api/src/externals/memory.rs
@@ -137,9 +137,6 @@ impl Memory {
         self.0
             .try_copy(&store)
             .map(|new_memory| Self::new_from_existing(new_store, new_memory.into()))
-            .ok_or_else(|| {
-                MemoryError::Generic("memory is not clonable or could not be copied".to_string())
-            })
     }
 
     pub(crate) fn from_vm_extern(store: &mut impl AsStoreMut, vm_extern: VMExternMemory) -> Self {
@@ -152,7 +149,7 @@ impl Memory {
     }
 
     /// Attempts to clone this memory (if its clonable)
-    pub fn try_clone(&self, store: &impl AsStoreRef) -> Option<VMMemory> {
+    pub fn try_clone(&self, store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
         self.0.try_clone(store)
     }
 
@@ -172,23 +169,6 @@ impl Memory {
         self.0
             .try_clone(&store)
             .map(|new_memory| Self::new_from_existing(new_store, new_memory))
-            .ok_or_else(|| MemoryError::Generic("memory is not clonable".to_string()))
-    }
-
-    /// Attempts to clone this memory (if its clonable) in a new store
-    /// (cloned memory will be shared between those that clone it)
-    #[deprecated = "use `share_in_store` or `copy_to_store` instead"]
-    pub fn duplicate_in_store(
-        &self,
-        store: &impl AsStoreRef,
-        new_store: &mut impl AsStoreMut,
-    ) -> Option<Self> {
-        if !self.ty(store).shared {
-            // We should only be able to duplicate in a new store if the memory is shared
-            return None;
-        }
-        #[allow(deprecated)]
-        self.0.duplicate_in_store(store, new_store).map(Self)
     }
 
     /// To `VMExtern`.

diff --git a/lib/api/src/js/externals/memory.rs b/lib/api/src/js/externals/memory.rs
@@ -142,25 +142,15 @@ impl Memory {
 
     /// Cloning memory will create another reference to the same memory that
     /// can be put into a new store
-    pub fn try_clone(&self, _store: &impl AsStoreRef) -> Option<VMMemory> {
+    pub fn try_clone(&self, _store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
         self.handle.try_clone()
     }
 
     /// Copying the memory will actually copy all the bytes in the memory to
     /// a identical byte copy of the original that can be put into a new store
-    pub fn try_copy(&self, store: &impl AsStoreRef) -> Option<VMMemory> {
-        self.try_clone(store).and_then(|mut mem| mem.copy().ok())
-    }
-
-    #[deprecated = "use `try_clone` and `try_copy` instead"]
-    pub fn duplicate_in_store(
-        &self,
-        store: &impl AsStoreRef,
-        new_store: &mut impl AsStoreMut,
-    ) -> Option<Self> {
-        self.try_clone(&store)
-            .and_then(|mut memory| memory.duplicate().ok())
-            .map(|new_memory| Self::new_from_existing(new_store, new_memory.into()))
+    pub fn try_copy(&self, store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
+        let cloned = self.try_clone(store)?;
+        mem.copy()
     }
 
     pub fn is_from_store(&self, _store: &impl AsStoreRef) -> bool {

diff --git a/lib/api/src/js/vm.rs b/lib/api/src/js/vm.rs
@@ -54,8 +54,8 @@ impl VMMemory {
     }
 
     /// Attempts to clone this memory (if its clonable)
-    pub(crate) fn try_clone(&self) -> Option<VMMemory> {
-        Some(self.clone())
+    pub(crate) fn try_clone(&self) -> Result<VMMemory, MemoryError> {
+        Ok(self.clone())
     }
 
     /// Copies this memory to a new memory

diff --git a/lib/api/src/jsc/externals/memory.rs b/lib/api/src/jsc/externals/memory.rs
@@ -172,26 +172,15 @@ impl Memory {
 
     /// Cloning memory will create another reference to the same memory that
     /// can be put into a new store
-    pub fn try_clone(&self, _store: &impl AsStoreRef) -> Option<VMMemory> {
+    pub fn try_clone(&self, _store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
         self.handle.try_clone()
     }
 
     /// Copying the memory will actually copy all the bytes in the memory to
     /// a identical byte copy of the original that can be put into a new store
-    pub fn try_copy(&self, store: &impl AsStoreRef) -> Option<VMMemory> {
-        self.try_clone(store)
-            .and_then(|mut mem| mem.copy(store).ok())
-    }
-
-    #[deprecated = "use `try_clone` and `try_copy` instead"]
-    pub fn duplicate_in_store(
-        &self,
-        store: &impl AsStoreRef,
-        new_store: &mut impl AsStoreMut,
-    ) -> Option<Self> {
-        self.try_clone(&store)
-            .and_then(|mut memory| memory.duplicate(&store).ok())
-            .map(|new_memory| Self::new_from_existing(new_store, new_memory.into()))
+    pub fn try_copy(&self, store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
+        let cloned = self.try_clone(store)?;
+        mem.copy(store)
     }
 
     pub fn is_from_store(&self, _store: &impl AsStoreRef) -> bool {

diff --git a/lib/api/src/jsc/vm.rs b/lib/api/src/jsc/vm.rs
@@ -45,8 +45,8 @@ impl VMMemory {
     }
 
     /// Attempts to clone this memory (if its clonable)
-    pub(crate) fn try_clone(&self) -> Option<VMMemory> {
-        Some(self.clone())
+    pub(crate) fn try_clone(&self) -> Result<VMMemory, MemoryError> {
+        Ok(self.clone())
     }
 
     /// Copies this memory to a new memory

diff --git a/lib/api/src/sys/externals/memory.rs b/lib/api/src/sys/externals/memory.rs
@@ -65,26 +65,20 @@ impl Memory {
 
     /// Cloning memory will create another reference to the same memory that
     /// can be put into a new store
-    pub fn try_clone(&self, store: &impl AsStoreRef) -> Option<VMMemory> {
+    pub fn try_clone(&self, store: &impl AsStoreRef) -> Result<VMMemory, MemoryError> {
         let mem = self.handle.get(store.as_store_ref().objects());
-        mem.try_clone().map(|mem| mem.into())
+        let cloned = mem.try_clone()?;
+        Ok(cloned.into())
     }
 
     /// Copying the memory will actually copy all the bytes in the memory to
     /// a identical byte copy of the original that can be put into a new store
-    pub fn try_copy(&self, store: &impl AsStoreRef) -> Option<Box<dyn LinearMemory + 'static>> {
-        self.try_clone(store).and_then(|mut mem| mem.copy().ok())
-    }
-
-    #[deprecated = "use `try_clone` and `try_copy` instead"]
-    pub fn duplicate_in_store(
+    pub fn try_copy(
         &self,
         store: &impl AsStoreRef,
-        new_store: &mut impl AsStoreMut,
-    ) -> Option<Self> {
-        self.try_clone(&store)
-            .and_then(|mut memory| memory.copy().ok())
-            .map(|new_memory| Self::new_from_existing(new_store, new_memory.into()))
+    ) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+        let mut mem = self.try_clone(store)?;
+        mem.copy()
     }
 
     /// To `VMExtern`.

diff --git a/lib/api/src/sys/tunables.rs b/lib/api/src/sys/tunables.rs
@@ -120,8 +120,10 @@ mod tests {
             }
         }
 
-        fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
-            None
+        fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+            Err(MemoryError::InvalidMemory {
+                reason: "VMTinyMemory can not be cloned".to_string(),
+            })
         }
 
         fn copy(&mut self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {

diff --git a/lib/sys-utils/src/memory/fd_memory/memories.rs b/lib/sys-utils/src/memory/fd_memory/memories.rs
@@ -343,9 +343,8 @@ impl LinearMemory for VMOwnedMemory {
     }
 
     /// Owned memory can not be cloned (this will always return None)
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
-        tracing::warn!("trying to clone owned memory");
-        None
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+        Err(MemoryError::MemoryNotShared)
     }
 
     /// Copies this memory to a new memory
@@ -440,8 +439,8 @@ impl LinearMemory for VMSharedMemory {
     }
 
     /// Shared memory can always be cloned
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
-        Some(Box::new(self.clone()))
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+        Ok(Box::new(self.clone()))
     }
 
     /// Copies this memory to a new memory
@@ -515,7 +514,7 @@ impl LinearMemory for VMMemory {
     }
 
     /// Attempts to clone this memory (if its clonable)
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
         self.0.try_clone()
     }
 

diff --git a/lib/types/src/error.rs b/lib/types/src/error.rs
@@ -47,6 +47,7 @@ pub enum DeserializeError {
 
 /// Error type describing things that can go wrong when operating on Wasm Memories.
 #[derive(Error, Debug, Clone, PartialEq, Eq, Hash)]
+#[non_exhaustive]
 pub enum MemoryError {
     /// Low level error with mmap.
     #[error("Error when allocating memory: {0}")]
@@ -60,7 +61,7 @@ pub enum MemoryError {
         /// The attempted amount to grow by in pages.
         attempted_delta: Pages,
     },
-    /// The operation would cause the size of the memory size exceed the maximum.
+    /// Invalid memory was provided.
     #[error("The memory is invalid because {}", reason)]
     InvalidMemory {
         /// The reason why the provided memory is invalid.
@@ -82,6 +83,9 @@ pub enum MemoryError {
         /// The number of pages requested as the maximum amount of memory.
         max_allowed: Pages,
     },
+    /// Returned when a shared memory is required, but the given memory is not shared.
+    #[error("The memory is not shared")]
+    MemoryNotShared,
     /// A user defined error value, used for error cases not listed above.
     #[error("A user-defined error occurred: {0}")]
     Generic(String),

diff --git a/lib/vm/src/memory.rs b/lib/vm/src/memory.rs
@@ -332,8 +332,8 @@ impl LinearMemory for VMOwnedMemory {
     }
 
     /// Owned memory can not be cloned (this will always return None)
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
-        None
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+        Err(MemoryError::MemoryNotShared)
     }
 
     /// Copies this memory to a new memory
@@ -429,8 +429,8 @@ impl LinearMemory for VMSharedMemory {
     }
 
     /// Shared memory can always be cloned
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
-        Some(Box::new(self.clone()))
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
+        Ok(Box::new(self.clone()))
     }
 
     /// Copies this memory to a new memory
@@ -506,7 +506,7 @@ impl LinearMemory for VMMemory {
     }
 
     /// Attempts to clone this memory (if its clonable)
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>> {
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError> {
         self.0.try_clone()
     }
 
@@ -637,7 +637,7 @@ where
     fn vmmemory(&self) -> NonNull<VMMemoryDefinition>;
 
     /// Attempts to clone this memory (if its clonable)
-    fn try_clone(&self) -> Option<Box<dyn LinearMemory + 'static>>;
+    fn try_clone(&self) -> Result<Box<dyn LinearMemory + 'static>, MemoryError>;
 
     #[doc(hidden)]
     /// # Safety