introduce a few Box::pins to limit future size

CHANGELOG.md

@@ -7,6 +7,7 @@
 - Added a field `adjust_instruction_addr: Option<bool>` to `RawFrame` to signal whether the
   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))
+- Use `moka` as an in-memory `Cacher` implementation. ([#979](https://github.com/getsentry/symbolicator/pull/979))
 
 ### Internal
 

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

@@ -367,7 +367,7 @@ impl<T: CacheItemRequest> Cacher<T> {
         let name = self.config.name();
         let key = request.get_cache_key();
 
-        let compute = async {
+        let compute = Box::pin(async {
             // cache_path is None when caching is disabled.
             if let Some(cache_dir) = self.config.cache_dir() {
                 let versions = std::iter::once(T::VERSIONS.current)
@@ -403,7 +403,7 @@ impl<T: CacheItemRequest> Cacher<T> {
             metric!(counter(&format!("caches.{name}.file.miss")) += 1);
 
             self.compute(request, &key, false).await
-        };
+        });
 
         self.cache.get_with_by_ref(&key, compute).await
     }
@@ -577,6 +577,28 @@ mod tests {
         }
     }
 
+    /// 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() {
+        test::setup();
+
+        let cache = Cache::from_config(
+            CacheName::Objects,
+            None,
+            None,
+            CacheConfig::from(CacheConfigs::default().derived),
+            Arc::new(AtomicIsize::new(1)),
+        )
+        .unwrap();
+        let cacher = Cacher::new(cache, Default::default());
+
+        let fut = cacher.compute_memoized(TestCacheItem::new("foo"));
+        let size = dbg!(std::mem::size_of_val(&fut));
+        assert!(size > 800 && size < 850);
+    }
+
     /// This test asserts that the cache is served from outdated cache files, and that a computation
     /// is being kicked off (and deduplicated) in the background
     #[tokio::test]

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/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/tests/integration/e2e.rs

@@ -494,6 +494,7 @@ fn get_files(path: impl AsRef<Path>) -> Vec<(String, u64)> {
 }
 
 /// Tests caching side-effects, like cache files written and hits to the symbol source.
+#[allow(clippy::if_same_then_else)]
 #[tokio::test]
 async fn test_basic_windows() {
     let (modules, stacktraces) = request_fixture();
@@ -559,7 +560,8 @@ async fn test_basic_windows() {
                     } else {
                         // we are downloading twice: once for the objects_meta request, and once
                         // again for the objects/symcache request
-                        (2, 1)
+                        // FIXME: well with in-memory caching, we are only requesting once
+                        (1, 1)
                     };
 
                     assert_eq!(&hits, &[