[PATCH 1/2] shared state: avoid borrow panics when removing listeners

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Removing a listener drops its callback while the listener list is
mutably borrowed. If the callback owns another observer of the same
state, destroying it tries to remove that observer under a second
mutable borrow and panics.

Release the borrow before destroying the removed callback so observer
teardown can unregister captured observers or drop other values whose
destructors access the state.

Fixes: f117efc2c4ce ("add experimental shared state object")
Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
---
 src/state/shared_state.rs | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/src/state/shared_state.rs b/src/state/shared_state.rs
index 2b5d7baf..d870db26 100644
--- a/src/state/shared_state.rs
+++ b/src/state/shared_state.rs
@@ -22,8 +22,8 @@ impl<T> SharedStateInner<T> {
         self.listeners.insert(cb)
     }
 
-    fn remove_listener(&mut self, key: usize) {
-        self.listeners.remove(key);
+    fn remove_listener(&mut self, key: usize) -> Callback<SharedState<T>> {
+        self.listeners.remove(key)
     }
 }
 
@@ -60,7 +60,10 @@ pub struct SharedStateObserver<T> {
 
 impl<T> Drop for SharedStateObserver<T> {
     fn drop(&mut self) {
-        self.inner.borrow_mut().remove_listener(self.key);
+        // Keep a named binding so the borrow ends before captured values are dropped. Using `let _`
+        // would drop the callback first, so destructors that borrow the state again could panic.
+        let callback = self.inner.borrow_mut().remove_listener(self.key);
+        drop(callback);
     }
 }
 
@@ -193,3 +196,29 @@ impl<T> Deref for SharedStateReadGuard<'_, T> {
         &self.borrowed_state
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::cell::Cell;
+
+    use super::*;
+
+    #[test]
+    fn removing_listener_can_drop_another_observer() {
+        let state = SharedState::new(());
+        let calls = Rc::new(Cell::new(0));
+        let first = state.add_listener({
+            let calls = Rc::clone(&calls);
+            move |_| calls.set(calls.get() + 1)
+        });
+        let second = state.add_listener(move |_| {
+            let _ = &first;
+        });
+
+        drop(state.write());
+        assert_eq!(calls.get(), 1);
+        drop(second);
+        drop(state.write());
+        assert_eq!(calls.get(), 1);
+    }
+}
-- 
2.47.3