feat(left-right-tlcache): cache entry invalidation

The existing cache relies on a single criteria to invalidate an
entry: if the ReadHandle<T> cannot be "entered", then it is invalid
and the provider needs to be asked for a refresh; which may fail or
not, depending on whether a factory for the key exists or not.
This behavior suffices to invalidate entries (read handles) when
the wrapped objects are removed or created in the provider. However,
providers (and hence caches) may have "alias" keys. An example is
a Fib object that can be identified by both an Id (master key) but
also a Vni. In such a case, the cache may itself have two cached
entries, pointing to the same readhandle. So far so good. However,
if such aliases get exchanged without the objects being torn down,
they will remain in the cache indefinitely, pointing to the wrong
object, because they will not be invalidated. In the example of a
Fib, suppose 2 Fibs with ids A and B. A is accessible via vni 10
and B only with its Id. At some point, there is a configuration
change such that Fib A no longer uses vni 10, but B is assigned
vni 10, and both Fibs continue to exist.Without a mechanism to
invalidate the entry with key vni = 10, the cache would return
a readhandle for A instead of B.

Invalidating such entries (vni 10 -> Rhandle(A)) can be tricky
because we don't want to poll the provider for a handle every time
(which would defeat the whole point of the cache) and because the
provider itself may be wrapped in left-right, meaning that
changes may not immediately appear in readers. Some mechanism is
needed to be able to tell if a cached entry points to the "right"
object, or has to be otherwise invalidated / refreshed.

This patch augments the implementation to address that case by:
  - defining the identity of a T (in ReadHandle<T>) with a trait
  - augmenting cached entries to store that identity, which may
    differ from the key used to access them
  - invalidating entries by detecting if their identity has
    changed. This is achieved by comparing the "cached identity"
    (the identity the cache believes the object accessed via the
    readhandle has) against the identity it sees when accessing
    the object.

The only additional requirement is implementing trait Identity for
T.

Signed-off-by: Fredi Raspall <[email protected]>

Author: Fredi-raspall

left-right-tlcache/src/lib.rs

@@ -47,6 +47,13 @@ pub trait ReadHandleProvider {
     fn get_factory(&self, key: &Self::Key) -> Option<&ReadHandleFactory<Self::Data>>;
 }
 
+/// Trait to determine the real identity of a `T` wrapped in left-right. That is,
+/// the identity of `T` in a `ReadHandle<T>`. This is needed to invalidate cache entries
+/// with keys that are alias of their identity.
+pub trait Identity<K> {
+    fn identity(&self) -> K;
+}
+
 #[derive(Debug, PartialEq, Error)]
 pub enum ReadHandleCacheError<K> {
     #[error("Reader not found for key {0}")]
@@ -55,12 +62,44 @@ pub enum ReadHandleCacheError<K> {
     NotAccessible(K),
 }
 
-pub struct ReadHandleCache<K: Hash + Eq, T> {
-    handles: RefCell<HashMap<K, Rc<ReadHandle<T>>, RandomState>>,
+/// An entry in a thread-local `ReadHandleCache<K,T>` to hold a `ReadHandle<T>`
+/// along with its identity, which may match the key to find it or not.
+struct ReadHandleEntry<T, K> {
+    rhandle: Rc<ReadHandle<T>>,
+    identity: K,
+}
+impl<T: Identity<K>, K: PartialEq> ReadHandleEntry<T, K> {
+    fn new(identity: K, rhandle: Rc<ReadHandle<T>>) -> Self {
+        Self { rhandle, identity }
+    }
+    /// A cached entry is valid if the ReadHandle<T> can be entered and the
+    /// object we'd enter into has the identity we believe it has.
+    fn is_valid(&self) -> bool {
+        if self.rhandle.was_dropped() {
+            return false;
+        }
+        match self.rhandle.enter() {
+            Some(t) => t.as_ref().identity() == self.identity,
+            None => false,
+        }
+    }
+}
+impl<T, K: Clone> Clone for ReadHandleEntry<T, K> {
+    fn clone(&self) -> Self {
+        Self {
+            rhandle: self.rhandle.clone(),
+            identity: self.identity.clone(),
+        }
+    }
+}
+
+pub struct ReadHandleCache<K: Hash + Eq + Clone, T> {
+    handles: RefCell<HashMap<K, ReadHandleEntry<T, K>, RandomState>>,
 }
 impl<K, T> ReadHandleCache<K, T>
 where
     K: Hash + Eq + Clone,
+    T: Identity<K>,
 {
     #[allow(clippy::new_without_default)]
     pub fn new() -> Self {
@@ -77,37 +116,32 @@ where
             let mut map = local.handles.borrow_mut();
 
             // cache has a valid handle for that key
-            if let Some(rhandle) = map.get(&key)
-                && !rhandle.was_dropped()
+            if let Some(entry) = map.get(&key)
+                && entry.is_valid()
             {
-                return Ok(Rc::clone(rhandle));
+                return Ok(Rc::clone(&entry.rhandle));
             }
 
-            // Either we've found a handle but was invalid or we did not find it.
-            // In either case, request a fresh handle to the provider, which may:
-            //    1) fail to provide one with that key
-            //    2) provide a valid one
-            //    3) provide an invalid one (e.g. if the `WriteHandle<T>` was dropped).
-            // We don't require providers to provide good readhandles/factories, nor
-            // want to return invalid handles to callers, so we validate them too.
-            // A valid readhandle is one where, at least now, can be 'entered'.
-
             let result = {
+                // get a factory for the key from the provider and build a fresh handle from it
                 let fresh = provider
                     .get_factory(&key)
                     .map(|factory| factory.handle())
                     .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?;
 
-                if fresh.was_dropped() {
-                    Err(ReadHandleCacheError::NotAccessible(key.clone()))
-                } else {
-                    let fresh = Rc::new(fresh);
-                    map.entry(key.clone())
-                        .and_modify(|e| *e = Rc::clone(&fresh))
-                        .or_insert_with(|| Rc::clone(&fresh));
+                // get master identity
+                let identity = fresh
+                    .enter()
+                    .as_ref()
+                    .ok_or_else(|| ReadHandleCacheError::NotAccessible(key.clone()))?
+                    .as_ref()
+                    .identity();
 
-                    Ok(fresh)
-                }
+                // store a new entry locally with a handle, its identity and the key
+                let rhandle = Rc::new(fresh);
+                let entry = ReadHandleEntry::new(identity, Rc::clone(&rhandle));
+                map.insert(key.clone(), entry);
+                Ok(rhandle)
             };
             if result.is_err() {
                 // clean-up cache on failure