Revert "ref: Use moka for Cacher implementation (#979)"

This reverts commit ff40e2c.
getsentry · Jan 30, 2023 · 59cab1f · 59cab1f
1 parent ff40e2c
commit 59cab1f
Show file tree

Hide file tree

Showing 8 changed files with 230 additions and 293 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,7 +8,6 @@
   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
 

diff --git a/crates/symbolicator-service/src/services/cache_key.rs b/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, Hash)]
+#[derive(Debug, Clone, Eq, Ord, PartialEq, PartialOrd)]
 pub struct CacheKey {
     pub cache_key: String,
 }

diff --git a/crates/symbolicator-service/src/services/cacher.rs b/crates/symbolicator-service/src/services/cacher.rs
diff --git a/crates/symbolicator-service/src/services/download/sentry.rs b/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 = Box::pin(async {
+        let init_future = 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

diff --git a/crates/symbolicator-service/src/services/objects/data_cache.rs b/crates/symbolicator-service/src/services/objects/data_cache.rs
@@ -264,7 +264,7 @@ mod tests {
     use symbolic::common::DebugId;
     use tempfile::TempDir;
 
-    async fn make_objects_actor(tempdir: &TempDir) -> ObjectsActor {
+    async fn 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 = make_objects_actor(&cachedir).await;
+        let objects_actor = objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -331,9 +331,6 @@ 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
@@ -354,7 +351,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = make_objects_actor(&cachedir).await;
+        let objects_actor = objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -383,9 +380,6 @@ 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
@@ -403,7 +397,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = make_objects_actor(&cachedir).await;
+        let objects_actor = objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -434,9 +428,6 @@ 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
@@ -454,7 +445,7 @@ mod tests {
 
         let hitcounter = test::Server::new();
         let cachedir = tempdir();
-        let objects_actor = make_objects_actor(&cachedir).await;
+        let objects_actor = objects_actor(&cachedir).await;
 
         let find_object = FindObject {
             filetypes: &[FileType::MachCode],
@@ -484,9 +475,6 @@ 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

diff --git a/crates/symbolicator-service/src/services/symbolication/process_minidump.rs b/crates/symbolicator-service/src/services/symbolication/process_minidump.rs
@@ -173,35 +173,36 @@ 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);
-        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 {
+        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()),
                     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
-        });
-        self.cficaches.get_with_by_ref(&key, load).await
+                };
+
+                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
     }
 }
 
@@ -517,74 +518,3 @@ 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);
-    }
-}
diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs
@@ -443,19 +443,20 @@ mod tests {
 
         // Create the symcache for the first time. Since the bcsymbolmap is not available, names in the
         // symcache will be obfuscated.
-        let symcache = symcache_actor
+        let owned_symcache = symcache_actor
             .fetch(fetch_symcache.clone())
             .await
             .cache
+            .ok()
             .unwrap();
 
-        let sl = symcache.get().lookup(0x5a75).next().unwrap();
+        let symcache = owned_symcache.get();
+        let sl = symcache.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(
@@ -471,30 +472,37 @@ 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 symcache = symcache_actor
+        let owned_symcache = symcache_actor
             .fetch(fetch_symcache.clone())
             .await
             .cache
+            .ok()
             .unwrap();
 
-        let sl = symcache.get().lookup(0x5a75).next().unwrap();
+        let symcache = owned_symcache.get();
+        let sl = symcache.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 symcache = symcache_actor.fetch(fetch_symcache).await.cache.unwrap();
+        let owned_symcache = symcache_actor
+            .fetch(fetch_symcache.clone())
+            .await
+            .cache
+            .ok()
+            .unwrap();
 
-        let sl = symcache.get().lookup(0x5a75).next().unwrap();
+        let symcache = owned_symcache.get();
+        let sl = symcache.lookup(0x5a75).next().unwrap();
         assert_eq!(
             sl.file().unwrap().full_path(),
             "/Users/philipphofmann/git-repos/sentry-cocoa/Sources/Sentry/SentryMessage.m"

diff --git a/crates/symbolicator-service/src/types/mod.rs b/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, Hash)]
+#[derive(Debug, Clone, Deserialize, Serialize, Eq, Ord, PartialEq, PartialOrd)]
 #[serde(untagged)]
 #[derive(Default)]
 pub enum Scope {