fix: lock two caches in address order in __eq__ and __ne__

src/policies/wrapped.rs

@@ -57,6 +57,31 @@ impl<P: PolicyExt> Wrapped<P> {
         pyo3::sync::MutexExt::lock_py_attached(&self.inner, py)
     }
 
+    /// Acquires the mutexes of two different caches and returns the guards as
+    /// `(self, other)`.
+    ///
+    /// The mutexes are always taken in address order, so `a == b` and `b == a`
+    /// on two threads cannot each hold one lock and wait for the other.
+    // https://github.com/awolverp/cachebox/issues/118
+    pub fn policy_pair_py_attached<'a>(
+        &'a self,
+        other: &'a Self,
+        py: pyo3::Python<'_>,
+    ) -> (
+        parking_lot::MutexGuard<'a, P>,
+        parking_lot::MutexGuard<'a, P>,
+    ) {
+        debug_assert!(!std::ptr::eq(self, other));
+
+        if (self as *const Self) < (other as *const Self) {
+            let first = self.policy_py_attached(py);
+            (first, other.policy_py_attached(py))
+        } else {
+            let first = other.policy_py_attached(py);
+            (self.policy_py_attached(py), first)
+        }
+    }
+
     /// Acquires the mutex only if it is free, returning `None` otherwise.
     ///
     /// For callers that must never wait for the lock, such as `__traverse__`:

src/pyclasses/cache.rs

@@ -514,8 +514,7 @@ impl PyCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -537,8 +536,7 @@ impl PyCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/fifocache.rs

@@ -516,8 +516,7 @@ impl PyFIFOCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -539,8 +538,7 @@ impl PyFIFOCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/lfucache.rs

@@ -535,8 +535,7 @@ impl PyLFUCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -558,8 +557,7 @@ impl PyLFUCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/lrucache.rs

@@ -543,8 +543,7 @@ impl PyLRUCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -566,8 +565,7 @@ impl PyLRUCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/rrcache.rs

@@ -516,8 +516,7 @@ impl PyRRCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -539,8 +538,7 @@ impl PyRRCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/ttlcache.rs

@@ -521,8 +521,7 @@ impl PyTTLCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -544,8 +543,7 @@ impl PyTTLCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

src/pyclasses/vttlcache.rs

@@ -495,8 +495,7 @@ impl PyVTTLCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy.py_eq(
             py,
@@ -518,8 +517,7 @@ impl PyVTTLCache {
         let self_inner = slf.0.get()?;
         let other_inner = other.0.get()?;
 
-        let self_policy = self_inner.policy_py_attached(py);
-        let other_policy = other_inner.policy_py_attached(py);
+        let (self_policy, other_policy) = self_inner.policy_pair_py_attached(other_inner, py);
 
         self_policy
             .py_eq(

tests/mixins.py

@@ -1221,6 +1221,59 @@ def worker(seed):
 print("ok")
 """
 
+COMPARE_WITH_ARGUMENTS_SWAPPED = """
+import faulthandler
+import sys
+import threading
+
+import cachebox
+
+name = sys.argv[1]
+cls = getattr(cachebox, name)
+# a hang prints every thread's stack before the parent's timeout
+faulthandler.dump_traceback_later(50, exit=True)
+# switch threads as often as possible, also inside Key.__eq__
+sys.setswitchinterval(1e-6)
+
+
+class Key:
+    def __init__(self, v):
+        self.v = v
+
+    def __hash__(self):
+        return self.v
+
+    def __eq__(self, other):
+        return self.v == other.v
+
+
+def make():
+    cache = cls(0, global_ttl=60) if name == "TTLCache" else cls(0)
+    for i in range(200):
+        cache[Key(i)] = i
+    return cache
+
+
+a, b = make(), make()
+
+
+def compare(x, y):
+    for _ in range(300):
+        assert x == y
+        assert not x != y
+
+
+threads = [
+    threading.Thread(target=compare, args=(a, b)),
+    threading.Thread(target=compare, args=(b, a)),
+]
+for t in threads:
+    t.start()
+for t in threads:
+    t.join()
+print("ok")
+"""
+
 
 class IssuesMixin(BaseMixin):
     def test_issue_5(self):
@@ -1302,6 +1355,30 @@ def test_lock_waiters_with_finalizers(self):
             done.stderr or f"exit code {done.returncode}"
         )
 
+    def test_compare_with_arguments_swapped(self):
+        # https://github.com/awolverp/cachebox/issues/118
+        #
+        # `a == b` locks both caches; with `b == a` running on another thread
+        # the locks were taken in opposite orders and both threads hung.
+        name = type(self.create_cache()).__name__
+
+        try:
+            done = subprocess.run(
+                [sys.executable, "-c", COMPARE_WITH_ARGUMENTS_SWAPPED, name],
+                capture_output=True,
+                text=True,
+                timeout=60,
+                check=False,
+            )
+        except subprocess.TimeoutExpired:
+            pytest.fail(
+                f"{name}: a == b and b == a on two threads never returned"
+            )
+
+        assert done.stdout.strip() == "ok", (
+            done.stderr or f"exit code {done.returncode}"
+        )
+
     def test_none_refcount(self):
         cache = self.create_cache(1000)