Remove (internal) borrow counter and use instead "locked" flag and st…

…rong reference counter
mlua-rs · Feb 14, 2025 · ae7cdcb · ae7cdcb
1 parent dacddfa
commit ae7cdcb
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 19 deletions.
diff --git a/src/userdata/cell.rs b/src/userdata/cell.rs
@@ -1,5 +1,5 @@
 use std::any::{type_name, TypeId};
-use std::cell::{Cell, RefCell, UnsafeCell};
+use std::cell::{RefCell, UnsafeCell};
 use std::fmt;
 use std::ops::{Deref, DerefMut};
 use std::os::raw::c_int;
@@ -91,20 +91,20 @@ impl<T> UserDataVariant<T> {
     }
 
     #[inline(always)]
-    fn raw_lock(&self) -> &RawLock {
+    fn strong_count(&self) -> usize {
         match self {
-            Self::Default(inner) => &inner.raw_lock,
+            Self::Default(inner) => XRc::strong_count(inner),
             #[cfg(feature = "serialize")]
-            Self::Serializable(inner) => &inner.raw_lock,
+            Self::Serializable(inner) => XRc::strong_count(inner),
         }
     }
 
     #[inline(always)]
-    fn borrow_count(&self) -> &Cell<usize> {
+    fn raw_lock(&self) -> &RawLock {
         match self {
-            Self::Default(inner) => &inner.borrow_count,
+            Self::Default(inner) => &inner.raw_lock,
             #[cfg(feature = "serialize")]
-            Self::Serializable(inner) => &inner.borrow_count,
+            Self::Serializable(inner) => &inner.raw_lock,
         }
     }
 
@@ -139,7 +139,6 @@ impl Serialize for UserDataStorage<()> {
 /// A type that provides interior mutability for a userdata value (thread-safe).
 pub(crate) struct UserDataCell<T> {
     raw_lock: RawLock,
-    borrow_count: Cell<usize>,
     value: UnsafeCell<T>,
 }
 
@@ -153,7 +152,6 @@ impl<T> UserDataCell<T> {
     fn new(value: T) -> Self {
         UserDataCell {
             raw_lock: RawLock::INIT,
-            borrow_count: Cell::new(0),
             value: UnsafeCell::new(value),
         }
     }
@@ -303,7 +301,6 @@ impl<T> Drop for UserDataBorrowRef<'_, T> {
     #[inline]
     fn drop(&mut self) {
         unsafe {
-            self.0.borrow_count().set(self.0.borrow_count().get() - 1);
             self.0.raw_lock().unlock_shared();
         }
     }
@@ -331,7 +328,6 @@ impl<'a, T> TryFrom<&'a UserDataVariant<T>> for UserDataBorrowRef<'a, T> {
         if !variant.raw_lock().try_lock_shared() {
             return Err(Error::UserDataBorrowError);
         }
-        variant.borrow_count().set(variant.borrow_count().get() + 1);
         Ok(UserDataBorrowRef(variant))
     }
 }
@@ -342,7 +338,6 @@ impl<T> Drop for UserDataBorrowMut<'_, T> {
     #[inline]
     fn drop(&mut self) {
         unsafe {
-            self.0.borrow_count().set(self.0.borrow_count().get() - 1);
             self.0.raw_lock().unlock_exclusive();
         }
     }
@@ -372,7 +367,6 @@ impl<'a, T> TryFrom<&'a UserDataVariant<T>> for UserDataBorrowMut<'a, T> {
         if !variant.raw_lock().try_lock_exclusive() {
             return Err(Error::UserDataBorrowMutError);
         }
-        variant.borrow_count().set(variant.borrow_count().get() + 1);
         Ok(UserDataBorrowMut(variant))
     }
 }
@@ -489,11 +483,15 @@ impl<T> UserDataStorage<T> {
         Self::Scoped(ScopedUserDataVariant::Boxed(RefCell::new(data)))
     }
 
+    /// Returns `true` if it's safe to destroy the container.
+    ///
+    /// It's safe to destroy the container if the reference count is greater than 1 or the lock is
+    /// not acquired.
     #[inline(always)]
-    pub(crate) fn is_borrowed(&self) -> bool {
+    pub(crate) fn is_safe_to_destroy(&self) -> bool {
         match self {
-            Self::Owned(variant) => variant.borrow_count().get() > 0,
-            Self::Scoped(_) => true,
+            Self::Owned(variant) => variant.strong_count() > 1 || !variant.raw_lock().is_locked(),
+            Self::Scoped(_) => false,
         }
     }
 

diff --git a/src/userdata/lock.rs b/src/userdata/lock.rs
@@ -1,6 +1,7 @@
 pub(crate) trait UserDataLock {
     const INIT: Self;
 
+    fn is_locked(&self) -> bool;
     fn try_lock_shared(&self) -> bool;
     fn try_lock_exclusive(&self) -> bool;
 
@@ -25,6 +26,11 @@ mod lock_impl {
         #[allow(clippy::declare_interior_mutable_const)]
         const INIT: Self = Cell::new(UNUSED);
 
+        #[inline(always)]
+        fn is_locked(&self) -> bool {
+            self.get() != UNUSED
+        }
+
         #[inline(always)]
         fn try_lock_shared(&self) -> bool {
             let flag = self.get().wrapping_add(1);
@@ -71,6 +77,11 @@ mod lock_impl {
         #[allow(clippy::declare_interior_mutable_const)]
         const INIT: Self = <Self as parking_lot::lock_api::RawRwLock>::INIT;
 
+        #[inline(always)]
+        fn is_locked(&self) -> bool {
+            RawRwLock::is_locked(self)
+        }
+
         #[inline(always)]
         fn try_lock_shared(&self) -> bool {
             RawRwLock::try_lock_shared(self)

diff --git a/src/userdata/util.rs b/src/userdata/util.rs
@@ -36,7 +36,7 @@ pub(crate) fn is_sync<T>() -> bool {
 
 pub(super) unsafe extern "C-unwind" fn userdata_destructor<T>(state: *mut ffi::lua_State) -> c_int {
     let ud = get_userdata::<UserDataStorage<T>>(state, -1);
-    if !(*ud).is_borrowed() {
+    if (*ud).is_safe_to_destroy() {
         take_userdata::<UserDataStorage<T>>(state);
         ffi::lua_pushboolean(state, 1);
     } else {

diff --git a/tests/userdata.rs b/tests/userdata.rs
@@ -410,7 +410,7 @@ fn test_userdata_destroy() -> Result<()> {
     let ud_ref = ud.borrow::<MyUserdata>()?;
     // With active `UserDataRef` this methods only marks userdata as destructed
     // without running destructor
-    ud.destroy()?;
+    ud.destroy().unwrap();
     assert_eq!(Arc::strong_count(&rc), 2);
     drop(ud_ref);
     assert_eq!(Arc::strong_count(&rc), 1);
@@ -419,7 +419,7 @@ fn test_userdata_destroy() -> Result<()> {
     let ud = lua.create_userdata(MyUserdata(rc.clone()))?;
     lua.globals().set("ud", &ud)?;
     lua.load("ud:try_destroy()").exec().unwrap();
-    ud.destroy()?;
+    ud.destroy().unwrap();
     assert_eq!(Arc::strong_count(&rc), 1);
 
     Ok(())