[pbs-devel] [PATCH proxmox-backup] pbs-tools: LruCache: implement Drop

[pbs-devel] [PATCH proxmox-backup] pbs-tools: LruCache: implement Drop

[Dominik Csapak]

this fixes the leaked memory for the cache, as we had only pointers
in the map/list which were freed, not the underlying chunks

moves the 'clear' implementation out of the trait bounds so that
Drop can reuse it

this is used e.g. for file download from a pxar

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-tools/src/lru_cache.rs | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/pbs-tools/src/lru_cache.rs b/pbs-tools/src/lru_cache.rs
index b1c0c6ac..fbd7673a 100644
--- a/pbs-tools/src/lru_cache.rs
+++ b/pbs-tools/src/lru_cache.rs
@@ -100,9 +100,25 @@ pub struct LruCache<K, V> {
     _marker: PhantomData<Box<CacheNode<K, V>>>,
 }
 
+impl<K, V> Drop for LruCache<K, V> {
+    fn drop (&mut self) {
+        self.clear();
+    }
+}
+
 // trivial: if our contents are Send, the whole cache is Send
 unsafe impl<K: Send, V: Send> Send for LruCache<K, V> {}
 
+impl<K, V> LruCache<K, V> {
+    /// Clear all the entries from the cache.
+    pub fn clear(&mut self) {
+        // This frees only the HashMap with the node pointers.
+        self.map.clear();
+        // This frees the actual nodes and resets the list head and tail.
+        self.list.clear();
+    }
+}
+
 impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
     /// Create LRU cache instance which holds up to `capacity` nodes at once.
     pub fn new(capacity: usize) -> Self {
@@ -115,14 +131,6 @@ impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
         }
     }
 
-    /// Clear all the entries from the cache.
-    pub fn clear(&mut self) {
-        // This frees only the HashMap with the node pointers.
-        self.map.clear();
-        // This frees the actual nodes and resets the list head and tail.
-        self.list.clear();
-    }
-
     /// Insert or update an entry identified by `key` with the given `value`.
     /// This entry is placed as the most recently used node at the head.
     pub fn insert(&mut self, key: K, value: V) {
-- 
2.30.2

[pbs-devel] applied: [PATCH proxmox-backup] pbs-tools: LruCache: implement Drop

[Wolfgang Bumiller]

applied, thanks

On Thu, Jan 20, 2022 at 11:16:08AM +0100, Dominik Csapak wrote:
> this fixes the leaked memory for the cache, as we had only pointers
> in the map/list which were freed, not the underlying chunks
> 
> moves the 'clear' implementation out of the trait bounds so that
> Drop can reuse it
> 
> this is used e.g. for file download from a pxar
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-tools/src/lru_cache.rs | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/pbs-tools/src/lru_cache.rs b/pbs-tools/src/lru_cache.rs
> index b1c0c6ac..fbd7673a 100644
> --- a/pbs-tools/src/lru_cache.rs
> +++ b/pbs-tools/src/lru_cache.rs
> @@ -100,9 +100,25 @@ pub struct LruCache<K, V> {
>      _marker: PhantomData<Box<CacheNode<K, V>>>,
>  }
>  
> +impl<K, V> Drop for LruCache<K, V> {
> +    fn drop (&mut self) {
> +        self.clear();
> +    }
> +}
> +
>  // trivial: if our contents are Send, the whole cache is Send
>  unsafe impl<K: Send, V: Send> Send for LruCache<K, V> {}
>  
> +impl<K, V> LruCache<K, V> {
> +    /// Clear all the entries from the cache.
> +    pub fn clear(&mut self) {
> +        // This frees only the HashMap with the node pointers.
> +        self.map.clear();
> +        // This frees the actual nodes and resets the list head and tail.
> +        self.list.clear();
> +    }
> +}
> +
>  impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
>      /// Create LRU cache instance which holds up to `capacity` nodes at once.
>      pub fn new(capacity: usize) -> Self {
> @@ -115,14 +131,6 @@ impl<K: std::cmp::Eq + std::hash::Hash + Copy, V> LruCache<K, V> {
>          }
>      }
>  
> -    /// Clear all the entries from the cache.
> -    pub fn clear(&mut self) {
> -        // This frees only the HashMap with the node pointers.
> -        self.map.clear();
> -        // This frees the actual nodes and resets the list head and tail.
> -        self.list.clear();
> -    }
> -
>      /// Insert or update an entry identified by `key` with the given `value`.
>      /// This entry is placed as the most recently used node at the head.
>      pub fn insert(&mut self, key: K, value: V) {
> -- 
> 2.30.2