ref: Use moka for Cacher implementation (#979)

Replaces the current request coalescing solution with `moka`.
This simplifies the channel creation and deduplication logic, as `moka`
does that already. This also gives us in-memory caches right now, even
though they are not hooked up to configuration yet.

CHANGELOG.md

@@ -8,6 +8,7 @@
   frame's instruction address needs to be adjusted for symbolication. ([#948](https://github.com/getsentry/symbolicator/pull/948))
 - Added offline mode and caching to `symbolicli`. ([#967](https://github.com/getsentry/symbolicator/pull/967),[#968](https://github.com/getsentry/symbolicator/pull/968))
 - Support PortablePDB embedded sources. ([#996](https://github.com/getsentry/symbolicator/pull/996))
+- Use `moka` as an in-memory `Cacher` implementation. ([#979](https://github.com/getsentry/symbolicator/pull/979))
 
 ### Internal
 

crates/symbolicator-service/src/services/cache_key.rs

@@ -5,7 +5,7 @@ use symbolicator_sources::RemoteFile;
 
 use crate::types::Scope;
 
-#[derive(Debug, Clone, Eq, Ord, PartialEq, PartialOrd)]
+#[derive(Debug, Clone, Eq, Ord, PartialEq, PartialOrd, Hash)]
 pub struct CacheKey {
     pub cache_key: String,
 }

crates/symbolicator-service/src/services/download/sentry.rs

@@ -98,7 +98,7 @@ impl SentryDownloader {
     /// If there are cached search results this skips the actual search.
     async fn cached_sentry_search(&self, query: SearchQuery) -> CacheEntry<Vec<SearchResult>> {
         let query_ = query.clone();
-        let init_future = async {
+        let init_future = Box::pin(async {
             tracing::debug!(
                 "Fetching list of Sentry debug files from {}",
                 &query_.index_url
@@ -112,7 +112,7 @@ impl SentryDownloader {
                 CancelOnDrop::new(self.runtime.spawn(future.bind_hub(sentry::Hub::current())));
 
             future.await.map_err(|_| CacheError::InternalError)?
-        };
+        });
         self.index_cache
             .get_with_if(query, init_future, |entry| entry.is_err())
             .await

crates/symbolicator-service/src/services/objects/data_cache.rs

@@ -264,7 +264,7 @@ mod tests {
     use symbolic::common::DebugId;
     use tempfile::TempDir;
 
-    async fn objects_actor(tempdir: &TempDir) -> ObjectsActor {
+    async fn make_objects_actor(tempdir: &TempDir) -> ObjectsActor {
         let meta_cache = Cache::from_config(
             CacheName::ObjectMeta,
             Some(tempdir.path().join("meta")),
@@ -299,7 +299,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = objects_actor(&cachedir).await;
+        let objects_actor = make_objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -331,6 +331,9 @@ mod tests {
             CacheError::DownloadError("500 Internal Server Error".into())
         );
         assert_eq!(hitcounter.accesses(), 3); // up to 3 tries on failure
+
+        // NOTE: creating a fresh instance to avoid in-memory cache
+        let objects_actor = make_objects_actor(&cachedir).await;
         let result = objects_actor
             .find(find_object.clone())
             .await
@@ -351,7 +354,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = objects_actor(&cachedir).await;
+        let objects_actor = make_objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -380,6 +383,9 @@ mod tests {
             .unwrap_err();
         assert_eq!(result, CacheError::NotFound);
         assert_eq!(hitcounter.accesses(), 1);
+
+        // NOTE: creating a fresh instance to avoid in-memory cache
+        let objects_actor = make_objects_actor(&cachedir).await;
         let result = objects_actor
             .find(find_object.clone())
             .await
@@ -397,7 +403,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = objects_actor(&cachedir).await;
+        let objects_actor = make_objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -428,6 +434,9 @@ mod tests {
         assert_eq!(result, err);
         // XXX: why are we not trying this 3 times?
         assert_eq!(hitcounter.accesses(), 1);
+
+        // NOTE: creating a fresh instance to avoid in-memory cache
+        let objects_actor = make_objects_actor(&cachedir).await;
         let result = objects_actor
             .find(find_object.clone())
             .await
@@ -445,7 +454,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = objects_actor(&cachedir).await;
+        let objects_actor = make_objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -475,6 +484,9 @@ mod tests {
             .unwrap_err();
         assert_eq!(result, err);
         assert_eq!(hitcounter.accesses(), 1);
+
+        // NOTE: creating a fresh instance to avoid in-memory cache
+        let objects_actor = make_objects_actor(&cachedir).await;
         let result = objects_actor
             .find(find_object.clone())
             .await

crates/symbolicator-service/src/services/symbolication/process_minidump.rs

@@ -173,36 +173,35 @@ impl SymbolicatorSymbolProvider {
     /// Fetches CFI for the given module, parses it into a `SymbolFile`, and stores it internally.
     async fn load_cfi_module(&self, module: &(dyn Module + Sync)) -> FetchedCfiCache {
         let key = LookupKey::new(module);
-        self.cficaches
-            .get_with_by_ref(&key, async {
-                let sources = self.sources.clone();
-                let scope = self.scope.clone();
-
-                let identifier = ObjectId {
-                    code_id: key.code_id.clone(),
-                    code_file: Some(module.code_file().into_owned()),
-                    debug_id: key.debug_id,
-                    debug_file: module
-                        .debug_file()
-                        .map(|debug_file| debug_file.into_owned()),
+        let load = Box::pin(async {
+            let sources = self.sources.clone();
+            let scope = self.scope.clone();
+
+            let identifier = ObjectId {
+                code_id: key.code_id.clone(),
+                code_file: Some(module.code_file().into_owned()),
+                debug_id: key.debug_id,
+                debug_file: module
+                    .debug_file()
+                    .map(|debug_file| debug_file.into_owned()),
+                object_type: self.object_type,
+            };
+
+            self.cficache_actor
+                .fetch(FetchCfiCache {
                     object_type: self.object_type,
-                };
-
-                self.cficache_actor
-                    .fetch(FetchCfiCache {
-                        object_type: self.object_type,
-                        identifier,
-                        sources,
-                        scope,
-                    })
-                    // NOTE: this `bind_hub` is important!
-                    // `load_cfi_module` is being called concurrently from `rust-minidump` via
-                    // `join_all`. We do need proper isolation of any async task that might
-                    // manipulate any Sentry scope.
-                    .bind_hub(Hub::new_from_top(Hub::current()))
-                    .await
-            })
-            .await
+                    identifier,
+                    sources,
+                    scope,
+                })
+                // NOTE: this `bind_hub` is important!
+                // `load_cfi_module` is being called concurrently from `rust-minidump` via
+                // `join_all`. We do need proper isolation of any async task that might
+                // manipulate any Sentry scope.
+                .bind_hub(Hub::new_from_top(Hub::current()))
+                .await
+        });
+        self.cficaches.get_with_by_ref(&key, load).await
     }
 }
 
@@ -518,3 +517,74 @@ fn normalize_minidump_os_name(os: Os) -> &'static str {
         Os::Unknown(_) => "",
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::services::create_service;
+    use crate::services::objects::{FindObject, ObjectPurpose};
+    use crate::services::ppdb_caches::FetchPortablePdbCache;
+    use crate::services::symcaches::FetchSymCache;
+
+    use super::*;
+
+    /// Tests that the size of the `compute_memoized` future does not grow out of bounds.
+    /// See <https://github.com/moka-rs/moka/issues/212> for one of the main issues here.
+    /// The size assertion will naturally change with compiler, dependency and code changes.
+    #[tokio::test]
+    async fn future_size() {
+        let (sym, obj) =
+            create_service(&Default::default(), tokio::runtime::Handle::current()).unwrap();
+
+        let provider = SymbolicatorSymbolProvider::new(
+            Scope::Global,
+            Arc::from_iter([]),
+            sym.cficaches.clone(),
+            Default::default(),
+        );
+
+        let module = ("foo", DebugId::nil());
+        let fut = provider.load_cfi_module(&module);
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 850 && size < 900);
+
+        let req = FindObject {
+            filetypes: &[],
+            purpose: ObjectPurpose::Debug,
+            identifier: Default::default(),
+            sources: Arc::from_iter([]),
+            scope: Scope::Global,
+        };
+        let fut = obj.find(req);
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 4800 && size < 4900);
+
+        let req = FetchCfiCache {
+            object_type: Default::default(),
+            identifier: Default::default(),
+            sources: Arc::from_iter([]),
+            scope: Scope::Global,
+        };
+        let fut = sym.cficaches.fetch(req);
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 5200 && size < 5300);
+
+        let req = FetchPortablePdbCache {
+            identifier: Default::default(),
+            sources: Arc::from_iter([]),
+            scope: Scope::Global,
+        };
+        let fut = sym.ppdb_caches.fetch(req);
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 5200 && size < 5300);
+
+        let req = FetchSymCache {
+            object_type: Default::default(),
+            identifier: Default::default(),
+            sources: Arc::from_iter([]),
+            scope: Scope::Global,
+        };
+        let fut = sym.symcaches.fetch(req);
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 11200 && size < 11300);
+    }
+}

crates/symbolicator-service/src/services/symcaches/mod.rs

@@ -443,20 +443,19 @@ mod tests {
 
         // Create the symcache for the first time. Since the bcsymbolmap is not available, names in the
         // symcache will be obfuscated.
-        let owned_symcache = symcache_actor
+        let symcache = symcache_actor
             .fetch(fetch_symcache.clone())
             .await
             .cache
-            .ok()
             .unwrap();
 
-        let symcache = owned_symcache.get();
-        let sl = symcache.lookup(0x5a75).next().unwrap();
+        let sl = symcache.get().lookup(0x5a75).next().unwrap();
         assert_eq!(
             sl.file().unwrap().full_path(),
             "__hidden#41_/__hidden#41_/__hidden#42_"
         );
         assert_eq!(sl.function().name(), "__hidden#0_");
+        drop(symcache);
 
         // Copy the plist and bcsymbolmap to the temporary symbol directory so that the SymCacheActor can find them.
         fs::copy(
@@ -472,37 +471,30 @@ mod tests {
         .unwrap();
 
         // Create the symcache for the second time. Even though the bcsymbolmap is now available, its absence should
-        // still be cached and the SymcacheActor should make no attempt to download it. Therefore, the names should
+        // still be cached and the SymCacheActor should make no attempt to download it. Therefore, the names should
         // be obfuscated like before.
-        let owned_symcache = symcache_actor
+        let symcache = symcache_actor
             .fetch(fetch_symcache.clone())
             .await
             .cache
-            .ok()
             .unwrap();
 
-        let symcache = owned_symcache.get();
-        let sl = symcache.lookup(0x5a75).next().unwrap();
+        let sl = symcache.get().lookup(0x5a75).next().unwrap();
         assert_eq!(
             sl.file().unwrap().full_path(),
             "__hidden#41_/__hidden#41_/__hidden#42_"
         );
         assert_eq!(sl.function().name(), "__hidden#0_");
+        drop(symcache);
 
         // Sleep long enough for the negative cache entry to become invalid.
         std::thread::sleep(TIMEOUT);
 
         // Create the symcache for the third time. This time, the bcsymbolmap is downloaded and the names in the
         // symcache are unobfuscated.
-        let owned_symcache = symcache_actor
-            .fetch(fetch_symcache.clone())
-            .await
-            .cache
-            .ok()
-            .unwrap();
+        let symcache = symcache_actor.fetch(fetch_symcache).await.cache.unwrap();
 
-        let symcache = owned_symcache.get();
-        let sl = symcache.lookup(0x5a75).next().unwrap();
+        let sl = symcache.get().lookup(0x5a75).next().unwrap();
         assert_eq!(
             sl.file().unwrap().full_path(),
             "/Users/philipphofmann/git-repos/sentry-cocoa/Sources/Sentry/SentryMessage.m"

crates/symbolicator-service/src/types/mod.rs

@@ -31,7 +31,7 @@ pub struct Signal(pub u32);
 /// Based on scopes, access to debug files that have been cached is determined. If a file comes from
 /// a public source, it can be used for any symbolication request. Otherwise, the symbolication
 /// request must match the scope of a file.
-#[derive(Debug, Clone, Deserialize, Serialize, Eq, Ord, PartialEq, PartialOrd)]
+#[derive(Debug, Clone, Deserialize, Serialize, Eq, Ord, PartialEq, PartialOrd, Hash)]
 #[serde(untagged)]
 #[derive(Default)]
 pub enum Scope {