From 9abf4d657307340bbd39500dd27fe5a3189aacdf Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Dec 2022 20:23:38 +0100 Subject: [PATCH 01/14] Add CacheEntry enum --- crates/symbolicator-service/src/cache.rs | 220 +++++++++++++++++++++++ 1 file changed, 220 insertions(+) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index 381177b4a..2321f00ee 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -10,6 +10,7 @@ use std::time::{Duration, SystemTime}; use anyhow::{anyhow, Result}; use filetime::FileTime; +use humantime_serde::re::humantime::{format_duration, parse_duration}; use serde::{Deserialize, Serialize}; use symbolic::common::ByteView; use tempfile::NamedTempFile; @@ -119,6 +120,174 @@ impl CacheStatus { } } +/// A cache entry that either contains an object of type `T` +/// or one of several error conditions. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CacheEntry { + /// A valid cache entry containing an instance of `T`. + AllGood(T), + /// The object was not found at the remote location. + NotFound, + /// The object could not be fetched from the remote source due to missing + /// permissions. + /// + /// The attached string contains the remote source's response. + PermissionDenied(String), // => whatever the server returned + /// The object could not be fetched from the remote source due to a timeout. + Timeout(Duration), // => should we return the duration after which we timed out? + /// The object could not be fetched from the remote source due to another problem, + /// like connection loss, DNS resolution, or a 5xx server response. + /// + /// The attached string contains the remote source's response. + DownloadError(String), + /// The object was fetched successfully, but is invalid in some way. + /// + /// For example, this could result from an unsupported object file or an error + /// during symcache conversion + Malformed(String), + /// An unexpected error in symbolicator itself. + InternalError, +} + +impl CacheEntry { + const PERMISSION_DENIED_MARKER: &[u8] = b"permissiondenied"; + const TIMEOUT_MARKER: &[u8] = b"timeout"; + const DOWNLOAD_ERROR_MARKER: &[u8] = b"downloaderror"; + const MALFORMED_MARKER: &[u8] = b"malformed"; + const INTERNAL_ERROR_MARKER: &[u8] = b"internalerror"; + + pub async fn write(&self, file: &mut File) -> Result<(), io::Error> { + file.rewind().await?; + match self { + CacheEntry::AllGood(_entry) => {} + CacheEntry::NotFound => { + file.set_len(0).await?; + } + CacheEntry::PermissionDenied(details) => { + file.write_all(Self::PERMISSION_DENIED_MARKER).await?; + file.write_all(details.as_bytes()).await?; + } + CacheEntry::Timeout(duration) => { + file.write_all(Self::TIMEOUT_MARKER).await?; + file.write_all(format_duration(*duration).to_string().as_bytes()) + .await?; + } + CacheEntry::DownloadError(details) => { + file.write_all(Self::DOWNLOAD_ERROR_MARKER).await?; + file.write_all(details.as_bytes()).await?; + } + CacheEntry::Malformed(details) => { + file.write_all(Self::MALFORMED_MARKER).await?; + file.write_all(details.as_bytes()).await?; + } + CacheEntry::InternalError => { + file.write_all(Self::INTERNAL_ERROR_MARKER).await?; + } + } + + let new_len = file.stream_position().await?; + file.set_len(new_len).await?; + Ok(()) + } + + pub fn map(self, f: F) -> CacheEntry + where + F: FnOnce(T) -> U, + { + match self { + Self::AllGood(entry) => CacheEntry::AllGood(f(entry)), + Self::NotFound => CacheEntry::NotFound, + Self::PermissionDenied(details) => CacheEntry::PermissionDenied(details), + Self::Timeout(duration) => CacheEntry::Timeout(duration), + Self::DownloadError(details) => CacheEntry::DownloadError(details), + Self::Malformed(details) => CacheEntry::Malformed(details), + Self::InternalError => CacheEntry::InternalError, + } + } + + pub fn try_map(self, f: F) -> CacheEntry + where + E: std::error::Error, + F: FnOnce(T) -> Result, + { + match self { + Self::AllGood(entry) => match f(entry) { + Ok(new_entry) => CacheEntry::AllGood(new_entry), + Err(e) => { + tracing::error!(error = %e); + CacheEntry::InternalError + } + }, + Self::NotFound => CacheEntry::NotFound, + Self::PermissionDenied(details) => CacheEntry::PermissionDenied(details), + Self::Timeout(duration) => CacheEntry::Timeout(duration), + Self::DownloadError(details) => CacheEntry::DownloadError(details), + Self::Malformed(details) => CacheEntry::Malformed(details), + Self::InternalError => CacheEntry::InternalError, + } + } + + pub fn all_good(self) -> Option { + match self { + Self::AllGood(entry) => Some(entry), + _ => None, + } + } + + pub fn is_not_found(&self) -> bool { + matches!(self, Self::NotFound) + } +} + +impl CacheEntry> { + pub fn from_bytes(bytes: ByteView<'static>) -> Self { + if let Some(raw_message) = bytes.strip_prefix(Self::PERMISSION_DENIED_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Self::PermissionDenied(err_msg.into_owned()) + } else if let Some(raw_duration) = bytes.strip_prefix(Self::TIMEOUT_MARKER) { + let raw_duration = String::from_utf8_lossy(raw_duration); + match parse_duration(&raw_duration) { + Ok(duration) => Self::Timeout(duration), + Err(e) => { + tracing::error!(error = %e, "Failed to read timeout duration"); + Self::InternalError + } + } + } else if let Some(raw_message) = bytes.strip_prefix(Self::DOWNLOAD_ERROR_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Self::DownloadError(err_msg.into_owned()) + } else if let Some(raw_message) = bytes.strip_prefix(Self::MALFORMED_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Self::Malformed(err_msg.into_owned()) + } else if bytes.as_ref() == Self::INTERNAL_ERROR_MARKER { + Self::InternalError + } else if bytes.is_empty() { + Self::NotFound + } else { + Self::AllGood(bytes) + } + } +} + +impl From<(CacheStatus, ByteView<'static>)> for CacheEntry> { + fn from((status, content): (CacheStatus, ByteView<'static>)) -> Self { + match status { + CacheStatus::Positive => Self::AllGood(content), + CacheStatus::Negative => Self::NotFound, + CacheStatus::Malformed(s) => Self::Malformed(s), + CacheStatus::CacheSpecificError(message) => { + if let Some(details) = message.strip_prefix("missing permissions for file") { + Self::PermissionDenied(details.to_string()) + } else if message == "download was cancelled" { + Self::Timeout(Duration::from_secs(0)) + } else { + Self::DownloadError(message) + } + } + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct FilesystemSharedCacheConfig { pub path: PathBuf, @@ -1599,4 +1768,55 @@ mod tests { SharedCacheBackendConfig::Filesystem(_) => panic!("wrong backend"), } } + + #[test] + fn test_cache_entry() { + fn read_cache_entry(bytes: &'static [u8]) -> CacheEntry { + CacheEntry::from_bytes(ByteView::from_slice(bytes)) + .map(|bv| String::from_utf8_lossy(bv.as_slice()).into_owned()) + } + + let not_found = b""; + + assert_eq!(read_cache_entry(not_found), CacheEntry::NotFound); + + let malformed = b"malformedDoesn't look like anything to me"; + + assert_eq!( + read_cache_entry(malformed), + CacheEntry::Malformed("Doesn't look like anything to me".into()) + ); + + let timeout = b"timeout4m33s"; + + assert_eq!( + read_cache_entry(timeout), + CacheEntry::Timeout(Duration::from_secs(273)) + ); + + let download_error = b"downloaderrorSomeone unplugged the internet"; + + assert_eq!( + read_cache_entry(download_error), + CacheEntry::DownloadError("Someone unplugged the internet".into()) + ); + + let permission_denied = b"permissiondeniedI'm sorry Dave, I'm afraid I can't do that"; + + assert_eq!( + read_cache_entry(permission_denied), + CacheEntry::PermissionDenied("I'm sorry Dave, I'm afraid I can't do that".into()) + ); + + let internal_error = b"internalerror"; + + assert_eq!(read_cache_entry(internal_error), CacheEntry::InternalError); + + let all_good = b"Not any of the error cases"; + + assert_eq!( + read_cache_entry(all_good), + CacheEntry::AllGood("Not any of the error cases".into()) + ); + } } From 185552cbac1b2f01e971c2aa50a03ccbb2e4878e Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Dec 2022 20:26:10 +0100 Subject: [PATCH 02/14] Add "owned" versions of symcache and ppdbcache --- .../symbolicator-service/src/services/ppdb_caches.rs | 10 +++++++++- .../symbolicator-service/src/services/symcaches/mod.rs | 8 +++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index 075ca7f9c..cce61bca7 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -7,7 +7,7 @@ use std::time::Duration; use futures::future::BoxFuture; use thiserror::Error; -use symbolic::common::ByteView; +use symbolic::common::{ByteView, SelfCell}; use symbolic::debuginfo::Object; use symbolic::ppdb::{PortablePdbCache, PortablePdbCacheConverter}; use symbolicator_sources::{FileType, ObjectId, SourceConfig}; @@ -44,6 +44,14 @@ const PPDB_CACHE_VERSIONS: CacheVersions = CacheVersions { fallbacks: &[], }; +pub type OwnedPortablePdbCache = SelfCell, PortablePdbCache<'static>>; + +fn parse_ppdb_cache_owned( + byteview: ByteView<'static>, +) -> Result { + SelfCell::try_new(byteview, |p| unsafe { PortablePdbCache::parse(&*p) }) +} + /// Errors happening while generating a symcache. #[derive(Debug, Error)] pub enum PortablePdbCacheError { diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index 61113f395..337302aba 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -8,7 +8,7 @@ use anyhow::Error; use futures::future::BoxFuture; use thiserror::Error; -use symbolic::common::{Arch, ByteView}; +use symbolic::common::{Arch, ByteView, SelfCell}; use symbolic::symcache::{self, SymCache, SymCacheConverter}; use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig}; @@ -72,6 +72,12 @@ const SYMCACHE_VERSIONS: CacheVersions = CacheVersions { }; static_assert!(symbolic::symcache::SYMCACHE_VERSION == 8); +pub type OwnedSymCache = SelfCell, SymCache<'static>>; + +fn parse_symcache_owned(byteview: ByteView<'static>) -> Result { + SelfCell::try_new(byteview, |p| unsafe { SymCache::parse(&*p) }) +} + /// Errors happening while generating a symcache. #[derive(Debug, Error)] pub enum SymCacheError { From 9ca8d3dd70d441afee08f9d779b8b62cb72e82af Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Dec 2022 20:26:44 +0100 Subject: [PATCH 03/14] Add conversions from errors to CacheEntry --- .../src/services/ppdb_caches.rs | 17 ++++++++++++++++- .../src/services/symcaches/mod.rs | 18 +++++++++++++++++- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index cce61bca7..b0b0e04ec 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -12,7 +12,7 @@ use symbolic::debuginfo::Object; use symbolic::ppdb::{PortablePdbCache, PortablePdbCacheConverter}; use symbolicator_sources::{FileType, ObjectId, SourceConfig}; -use crate::cache::{Cache, CacheStatus, ExpirationTime}; +use crate::cache::{Cache, CacheEntry, CacheStatus, ExpirationTime}; use crate::services::objects::ObjectError; use crate::types::{AllObjectCandidates, ObjectFeatures, ObjectUseInfo, Scope}; use crate::utils::futures::{m, measure}; @@ -76,6 +76,21 @@ pub enum PortablePdbCacheError { #[error("ppdb cache building took too long")] Timeout, } + +impl From<&PortablePdbCacheError> for CacheEntry { + fn from(error: &PortablePdbCacheError) -> Self { + match *error { + PortablePdbCacheError::Io(_) + | PortablePdbCacheError::Parsing(_) + | PortablePdbCacheError::PortablePdbParsing(_) + | PortablePdbCacheError::Writing(_) => Self::InternalError, + PortablePdbCacheError::Fetching(ref e) => Self::DownloadError(e.to_string()), + PortablePdbCacheError::Malformed => Self::Malformed(String::new()), + PortablePdbCacheError::Timeout => Self::Timeout(Duration::default()), + } + } +} + #[derive(Debug, Clone)] pub struct PortablePdbCacheFile { data: ByteView<'static>, diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index 337302aba..f0d077cd9 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -12,7 +12,7 @@ use symbolic::common::{Arch, ByteView, SelfCell}; use symbolic::symcache::{self, SymCache, SymCacheConverter}; use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig}; -use crate::cache::{Cache, CacheStatus, ExpirationTime}; +use crate::cache::{Cache, CacheEntry, CacheStatus, ExpirationTime}; use crate::services::bitcode::BitcodeService; use crate::services::cacher::{CacheItemRequest, CacheKey, CacheVersions, Cacher}; use crate::services::objects::{ @@ -109,6 +109,22 @@ pub enum SymCacheError { Timeout, } +impl From<&SymCacheError> for CacheEntry { + fn from(error: &SymCacheError) -> Self { + match error { + SymCacheError::Io(_) + | SymCacheError::Parsing(_) + | SymCacheError::Writing(_) + | SymCacheError::ObjectParsing(_) + | SymCacheError::BcSymbolMapError(_) + | SymCacheError::Il2cppError(_) => Self::InternalError, + SymCacheError::Fetching(ref e) => Self::DownloadError(e.to_string()), + SymCacheError::Malformed => Self::Malformed(String::new()), + SymCacheError::Timeout => Self::Timeout(Duration::default()), + } + } +} + #[derive(Clone, Debug)] pub struct SymCacheActor { symcaches: Arc>, From d7456fceae1f227742316e007f61a02505753d30 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Dec 2022 20:27:33 +0100 Subject: [PATCH 04/14] Change signatures of fetch methods This changes `SymCacheActor::fetch` and `PortablePdbCacheActor::fetch` so that they return a `CacheEntry` (plus auxiliary information, i.e. candidates and features). --- .../src/services/ppdb_caches.rs | 40 +++++++---- .../src/services/symcaches/mod.rs | 71 ++++++++++++++----- 2 files changed, 80 insertions(+), 31 deletions(-) diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index b0b0e04ec..ef3beb2cf 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -155,8 +155,12 @@ impl PortablePdbCacheActor { pub async fn fetch( &self, request: FetchPortablePdbCache, - ) -> Result, Arc> { - let FoundObject { meta, candidates } = self + ) -> ( + CacheEntry, + AllObjectCandidates, + ObjectFeatures, + ) { + let Ok(FoundObject { meta, candidates }) = self .objects .find(FindObject { filetypes: &[FileType::PortablePdb], @@ -165,26 +169,38 @@ impl PortablePdbCacheActor { scope: request.scope.clone(), purpose: ObjectPurpose::Debug, }) - .await - .map_err(|e| Arc::new(PortablePdbCacheError::Fetching(e)))?; + .await else { + return (CacheEntry::InternalError, AllObjectCandidates::default(), ObjectFeatures::default()) + }; match meta { Some(handle) => { - self.ppdb_caches + match self + .ppdb_caches .compute_memoized(FetchPortablePdbCacheInternal { request, objects_actor: self.objects.clone(), object_meta: handle, - candidates, + candidates: candidates.clone(), }) .await + { + Ok(ppdb_cache_file) => { + let PortablePdbCacheFile { + status, + data, + candidates, + features, + .. + } = Arc::try_unwrap(ppdb_cache_file).unwrap_or_else(|arc| (*arc).clone()); + let cache_entry = CacheEntry::from((status, data)); + let mapped = cache_entry.try_map(parse_ppdb_cache_owned); + (mapped, candidates, features) + } + Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), + } } - None => Ok(Arc::new(PortablePdbCacheFile { - data: ByteView::from_slice(b""), - status: CacheStatus::Negative, - candidates, - features: ObjectFeatures::default(), - })), + None => (CacheEntry::NotFound, candidates, ObjectFeatures::default()), } } } diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index f0d077cd9..8a3cbf924 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -340,8 +340,12 @@ impl SymCacheActor { pub async fn fetch( &self, request: FetchSymCache, - ) -> Result, Arc> { - let FoundObject { meta, candidates } = self + ) -> ( + CacheEntry, + AllObjectCandidates, + ObjectFeatures, + ) { + let Ok(FoundObject { meta, candidates }) = self .objects .find(FindObject { filetypes: FileType::from_object_type(request.object_type), @@ -350,8 +354,9 @@ impl SymCacheActor { scope: request.scope.clone(), purpose: ObjectPurpose::Debug, }) - .await - .map_err(|e| Arc::new(SymCacheError::Fetching(e)))?; + .await else { + return (CacheEntry::InternalError, AllObjectCandidates::default(), ObjectFeatures::default()) + }; match meta { Some(handle) => { @@ -397,23 +402,33 @@ impl SymCacheActor { il2cpp_handle, }; - self.symcaches + match self + .symcaches .compute_memoized(FetchSymCacheInternal { request, objects_actor: self.objects.clone(), secondary_sources, object_meta: handle, - candidates, + candidates: candidates.clone(), }) .await + { + Ok(symcache_file) => { + let SymCacheFile { + status, + data, + candidates, + features, + .. + } = Arc::try_unwrap(symcache_file).unwrap_or_else(|arc| (*arc).clone()); + let cache_entry = CacheEntry::from((status, data)); + let mapped = cache_entry.try_map(parse_symcache_owned); + (mapped, candidates, features) + } + Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), + } } - None => Ok(Arc::new(SymCacheFile { - data: ByteView::from_slice(b""), - features: ObjectFeatures::default(), - status: CacheStatus::Negative, - arch: Arch::Unknown, - candidates, - })), + None => (CacheEntry::NotFound, candidates, ObjectFeatures::default()), } } } @@ -600,8 +615,14 @@ mod tests { // Create the symcache for the first time. Since the bcsymbolmap is not available, names in the // symcache will be obfuscated. - let symcache_file = symcache_actor.fetch(fetch_symcache.clone()).await.unwrap(); - let symcache = symcache_file.parse().unwrap().unwrap(); + let owned_symcache = symcache_actor + .fetch(fetch_symcache.clone()) + .await + .0 + .all_good() + .unwrap(); + + let symcache = owned_symcache.get(); let sl = symcache.lookup(0x5a75).next().unwrap(); assert_eq!( sl.file().unwrap().full_path(), @@ -625,8 +646,14 @@ mod tests { // 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 // be obfuscated like before. - let symcache_file = symcache_actor.fetch(fetch_symcache.clone()).await.unwrap(); - let symcache = symcache_file.parse().unwrap().unwrap(); + let owned_symcache = symcache_actor + .fetch(fetch_symcache.clone()) + .await + .0 + .all_good() + .unwrap(); + + let symcache = owned_symcache.get(); let sl = symcache.lookup(0x5a75).next().unwrap(); assert_eq!( sl.file().unwrap().full_path(), @@ -639,8 +666,14 @@ mod tests { // Create the symcache for the third time. This time, the bcsymbolmap is downloaded and the names in the // symcache are unobfuscated. - let symcache_file = symcache_actor.fetch(fetch_symcache.clone()).await.unwrap(); - let symcache = symcache_file.parse().unwrap().unwrap(); + let owned_symcache = symcache_actor + .fetch(fetch_symcache.clone()) + .await + .0 + .all_good() + .unwrap(); + + let symcache = owned_symcache.get(); let sl = symcache.lookup(0x5a75).next().unwrap(); assert_eq!( sl.file().unwrap().full_path(), From 9ba6de9fe1b846b8a2f2e6c69e141f6786fcd140 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Thu, 1 Dec 2022 20:30:16 +0100 Subject: [PATCH 05/14] Refactor module lookup to use CacheEntry --- .../services/symbolication/module_lookup.rs | 124 ++++++++++-------- .../services/symbolication/symbolication.rs | 45 +++---- 2 files changed, 89 insertions(+), 80 deletions(-) diff --git a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs index c703f4b85..064aef084 100644 --- a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs +++ b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs @@ -9,18 +9,33 @@ use symbolic::common::{ByteView, SelfCell}; use symbolic::debuginfo::{Object, ObjectDebugSession}; use symbolicator_sources::{FileType, ObjectType, SourceConfig}; +use crate::cache::CacheEntry; use crate::services::objects::{FindObject, FoundObject, ObjectPurpose, ObjectsActor}; use crate::services::ppdb_caches::{ - FetchPortablePdbCache, PortablePdbCacheActor, PortablePdbCacheError, PortablePdbCacheFile, + FetchPortablePdbCache, OwnedPortablePdbCache, PortablePdbCacheActor, PortablePdbCacheError, }; -use crate::services::symcaches::{FetchSymCache, SymCacheActor, SymCacheError, SymCacheFile}; +use crate::services::symcaches::{FetchSymCache, OwnedSymCache, SymCacheActor, SymCacheError}; use crate::types::{ - CompleteObjectInfo, CompleteStacktrace, ObjectFileStatus, RawStacktrace, Scope, + AllObjectCandidates, CompleteObjectInfo, CompleteStacktrace, ObjectFeatures, ObjectFileStatus, + RawStacktrace, Scope, }; use crate::utils::addr::AddrMode; use super::object_id_from_object_info; +fn object_file_status_from_cache_entry(cache_entry: &CacheEntry) -> ObjectFileStatus { + match cache_entry { + CacheEntry::AllGood(_) => ObjectFileStatus::Found, + CacheEntry::NotFound => ObjectFileStatus::Missing, + CacheEntry::PermissionDenied(_) | CacheEntry::DownloadError(_) => { + ObjectFileStatus::FetchingFailed + } + CacheEntry::Timeout(_) => ObjectFileStatus::Timeout, + CacheEntry::Malformed(_) => ObjectFileStatus::Malformed, + CacheEntry::InternalError => ObjectFileStatus::Other, + } +} + #[derive(Debug, Error)] pub enum CacheFileError { #[error(transparent)] @@ -30,16 +45,23 @@ pub enum CacheFileError { } #[derive(Debug, Clone)] -pub enum CacheFile { - SymCache(Arc), - PortablePdbCache(Arc), +pub enum CacheFileEntry { + SymCache(OwnedSymCache), + PortablePdbCache(OwnedPortablePdbCache), +} + +#[derive(Debug, Clone)] +pub struct CacheFile { + file: CacheEntry, + candidates: AllObjectCandidates, + features: ObjectFeatures, } #[derive(Debug, Clone)] pub struct CacheLookupResult<'a> { pub module_index: usize, pub object_info: &'a CompleteObjectInfo, - pub cache: Option<&'a CacheFile>, + pub cache: &'a CacheEntry, pub relative_addr: Option, } @@ -72,7 +94,7 @@ pub struct SourceObject(SelfCell, Object<'static>>); struct ModuleEntry { module_index: usize, object_info: CompleteObjectInfo, - cache: Option, + cache: CacheEntry, source_object: Option, } @@ -94,7 +116,7 @@ impl ModuleLookup { .map(|(module_index, object_info)| ModuleEntry { module_index, object_info, - cache: None, + cache: CacheEntry::NotFound, source_object: None, }) .collect(); @@ -184,13 +206,17 @@ impl ModuleLookup { sources, scope, }; - - let ppdb_cache_result = match ppdb_cache_actor.fetch(request).await { - Ok(ppdb_cache) => Ok(CacheFile::PortablePdbCache(ppdb_cache)), - Err(e) => Err(e.as_ref().into()), - }; - - (idx, ppdb_cache_result) + let (ppdb_cache_entry, candidates, features) = + ppdb_cache_actor.fetch(request).await; + + ( + idx, + CacheFile { + file: ppdb_cache_entry.map(CacheFileEntry::PortablePdbCache), + candidates, + features, + }, + ) } _ => { let request = FetchSymCache { @@ -200,12 +226,17 @@ impl ModuleLookup { scope, }; - let symcache_result = match symcache_actor.fetch(request).await { - Ok(symcache) => Ok(CacheFile::SymCache(symcache)), - Err(e) => Err(e.as_ref().into()), - }; - - (idx, symcache_result) + let (symcache_entry, candidates, features) = + symcache_actor.fetch(request).await; + + ( + idx, + CacheFile { + file: symcache_entry.map(CacheFileEntry::SymCache), + candidates, + features, + }, + ) } } } @@ -213,39 +244,26 @@ impl ModuleLookup { Some(fut) }); - for (idx, cache_result) in future::join_all(futures).await { + for ( + idx, + CacheFile { + file, + candidates, + features, + }, + ) in future::join_all(futures).await + { if let Some(entry) = self.modules.get_mut(idx) { entry.object_info.arch = Default::default(); + entry.object_info.features.merge(features); + entry.object_info.candidates.merge(&candidates); + entry.object_info.debug_status = object_file_status_from_cache_entry(&file); - let (cache, status) = match cache_result { - Ok(cache) => match cache { - CacheFile::SymCache(ref symcache) => { - entry.object_info.arch = symcache.arch(); - entry.object_info.features.merge(symcache.features()); - entry.object_info.candidates.merge(symcache.candidates()); - - match symcache.parse() { - Ok(Some(_)) => (Some(cache), ObjectFileStatus::Found), - Ok(None) => (Some(cache), ObjectFileStatus::Missing), - Err(e) => (None, (&e).into()), - } - } - - CacheFile::PortablePdbCache(ref ppdb_cache) => { - entry.object_info.features.merge(ppdb_cache.features()); - entry.object_info.candidates.merge(ppdb_cache.candidates()); - match ppdb_cache.parse() { - Ok(Some(_)) => (Some(cache), ObjectFileStatus::Found), - Ok(None) => (Some(cache), ObjectFileStatus::Missing), - Err(e) => (None, (&e).into()), - } - } - }, - Err(e) => (None, e), - }; + if let CacheEntry::AllGood(CacheFileEntry::SymCache(ref symcache)) = file { + entry.object_info.arch = symcache.get().arch(); + } - entry.object_info.debug_status = status; - entry.cache = cache; + entry.cache = file; } } } @@ -333,7 +351,7 @@ impl ModuleLookup { CacheLookupResult { module_index: entry.module_index, object_info: &entry.object_info, - cache: entry.cache.as_ref(), + cache: &entry.cache, relative_addr, } }) @@ -523,6 +541,6 @@ mod tests { let lookup_result = lookup.lookup_cache(43, AddrMode::Abs).unwrap(); assert_eq!(lookup_result.module_index, 0); assert_eq!(lookup_result.object_info, &info); - assert!(lookup_result.cache.is_none()); + assert!(lookup_result.cache.is_not_found()); } } diff --git a/crates/symbolicator-service/src/services/symbolication/symbolication.rs b/crates/symbolicator-service/src/services/symbolication/symbolication.rs index a2d0a832e..772a8a63f 100644 --- a/crates/symbolicator-service/src/services/symbolication/symbolication.rs +++ b/crates/symbolicator-service/src/services/symbolication/symbolication.rs @@ -2,10 +2,11 @@ use std::sync::Arc; use symbolic::common::{split_path, DebugId, InstructionInfo, Language, Name}; use symbolic::demangle::{Demangle, DemangleOptions}; +use symbolic::ppdb::PortablePdbCache; +use symbolic::symcache::SymCache; use symbolicator_sources::{ObjectType, SourceConfig}; -use crate::services::ppdb_caches::PortablePdbCacheFile; -use crate::services::symcaches::SymCacheFile; +use crate::cache::CacheEntry; use crate::types::{ CompleteObjectInfo, CompleteStacktrace, CompletedSymbolicationResponse, FrameStatus, FrameTrust, ObjectFileStatus, RawFrame, RawStacktrace, Registers, Scope, Signal, @@ -13,7 +14,7 @@ use crate::types::{ }; use crate::utils::hex::HexValue; -use super::module_lookup::{CacheFile, CacheLookupResult, ModuleLookup}; +use super::module_lookup::{CacheFileEntry, CacheLookupResult, ModuleLookup}; use super::SymbolicationActor; impl SymbolicationActor { @@ -135,31 +136,27 @@ fn symbolicate_frame( frame.package = lookup_result.object_info.raw.code_file.clone(); match lookup_result.cache { - Some(CacheFile::SymCache(symcache)) => { - symbolicate_native_frame(symcache, lookup_result, registers, signal, frame, index) - } - Some(CacheFile::PortablePdbCache(ppdbcache)) => { - symbolicate_dotnet_frame(ppdbcache, frame, index) - } - None if lookup_result.object_info.debug_status == ObjectFileStatus::Malformed => { - Err(FrameStatus::Malformed) + CacheEntry::AllGood(CacheFileEntry::SymCache(symcache)) => symbolicate_native_frame( + symcache.get(), + lookup_result, + registers, + signal, + frame, + index, + ), + CacheEntry::AllGood(CacheFileEntry::PortablePdbCache(ppdb_cache)) => { + symbolicate_dotnet_frame(ppdb_cache.get(), frame, index) } - None => Err(FrameStatus::Missing), + CacheEntry::Malformed(_) => Err(FrameStatus::Malformed), + _ => Err(FrameStatus::Missing), } } fn symbolicate_dotnet_frame( - ppdbcache: &PortablePdbCacheFile, + ppdbcache: &PortablePdbCache, frame: &RawFrame, index: usize, ) -> Result, FrameStatus> { - tracing::trace!("Loading ppdbcache"); - let ppdbcache = match ppdbcache.parse() { - Ok(Some(x)) => x, - Ok(None) => return Err(FrameStatus::Missing), - Err(_) => return Err(FrameStatus::Malformed), - }; - // TODO: Add a new error variant for this? let function_idx = frame.function_id.ok_or(FrameStatus::MissingSymbol)?.0 as u32; let il_offset = frame.instruction_addr.0 as u32; @@ -186,19 +183,13 @@ fn symbolicate_dotnet_frame( } fn symbolicate_native_frame( - symcache: &SymCacheFile, + symcache: &SymCache, lookup_result: CacheLookupResult, registers: &Registers, signal: Option, frame: &RawFrame, index: usize, ) -> Result, FrameStatus> { - tracing::trace!("Loading symcache"); - let symcache = match symcache.parse() { - Ok(Some(x)) => x, - Ok(None) => return Err(FrameStatus::Missing), - Err(_) => return Err(FrameStatus::Malformed), - }; // get the relative caller address let relative_addr = if let Some(addr) = lookup_result.relative_addr { // heuristics currently are only supported when we can work with absolute addresses. From ce6d51b9b3c3f7bb1bf5c7d8fce73ea6faecbd91 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Fri, 2 Dec 2022 12:52:36 +0100 Subject: [PATCH 06/14] Don't read/write internal errors --- crates/symbolicator-service/src/cache.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index 2321f00ee..683fdbcca 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -154,7 +154,6 @@ impl CacheEntry { const TIMEOUT_MARKER: &[u8] = b"timeout"; const DOWNLOAD_ERROR_MARKER: &[u8] = b"downloaderror"; const MALFORMED_MARKER: &[u8] = b"malformed"; - const INTERNAL_ERROR_MARKER: &[u8] = b"internalerror"; pub async fn write(&self, file: &mut File) -> Result<(), io::Error> { file.rewind().await?; @@ -180,9 +179,7 @@ impl CacheEntry { file.write_all(Self::MALFORMED_MARKER).await?; file.write_all(details.as_bytes()).await?; } - CacheEntry::InternalError => { - file.write_all(Self::INTERNAL_ERROR_MARKER).await?; - } + CacheEntry::InternalError => {} } let new_len = file.stream_position().await?; @@ -259,8 +256,6 @@ impl CacheEntry> { } else if let Some(raw_message) = bytes.strip_prefix(Self::MALFORMED_MARKER) { let err_msg = String::from_utf8_lossy(raw_message); Self::Malformed(err_msg.into_owned()) - } else if bytes.as_ref() == Self::INTERNAL_ERROR_MARKER { - Self::InternalError } else if bytes.is_empty() { Self::NotFound } else { @@ -1808,10 +1803,6 @@ mod tests { CacheEntry::PermissionDenied("I'm sorry Dave, I'm afraid I can't do that".into()) ); - let internal_error = b"internalerror"; - - assert_eq!(read_cache_entry(internal_error), CacheEntry::InternalError); - let all_good = b"Not any of the error cases"; assert_eq!( From 4632f6c0ff667d8a4de6591a48802442b0074b33 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Fri, 2 Dec 2022 13:00:02 +0100 Subject: [PATCH 07/14] Log errors when converting from sym/ppdbcache --- .../src/services/ppdb_caches.rs | 22 ++++++++++---- .../src/services/symcaches/mod.rs | 30 +++++++++++++++---- 2 files changed, 41 insertions(+), 11 deletions(-) diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index ef3beb2cf..83af7784c 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -79,11 +79,23 @@ pub enum PortablePdbCacheError { impl From<&PortablePdbCacheError> for CacheEntry { fn from(error: &PortablePdbCacheError) -> Self { - match *error { - PortablePdbCacheError::Io(_) - | PortablePdbCacheError::Parsing(_) - | PortablePdbCacheError::PortablePdbParsing(_) - | PortablePdbCacheError::Writing(_) => Self::InternalError, + match error { + PortablePdbCacheError::Io(e) => { + tracing::error!(error = %e, "failed to write ppdb cache"); + Self::InternalError + } + PortablePdbCacheError::Parsing(e) => { + tracing::error!(error = %e, "failed to parse ppdb cache"); + Self::InternalError + } + PortablePdbCacheError::PortablePdbParsing(e) => { + tracing::error!(error = %e, "failed to parse portable pdb"); + Self::InternalError + } + PortablePdbCacheError::Writing(e) => { + tracing::error!(error = %e, "failed to write ppdb cache"); + Self::InternalError + } PortablePdbCacheError::Fetching(ref e) => Self::DownloadError(e.to_string()), PortablePdbCacheError::Malformed => Self::Malformed(String::new()), PortablePdbCacheError::Timeout => Self::Timeout(Duration::default()), diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index 8a3cbf924..5170c2e13 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -112,12 +112,30 @@ pub enum SymCacheError { impl From<&SymCacheError> for CacheEntry { fn from(error: &SymCacheError) -> Self { match error { - SymCacheError::Io(_) - | SymCacheError::Parsing(_) - | SymCacheError::Writing(_) - | SymCacheError::ObjectParsing(_) - | SymCacheError::BcSymbolMapError(_) - | SymCacheError::Il2cppError(_) => Self::InternalError, + SymCacheError::Io(e) => { + tracing::error!(error = %e, "failed to write symcache"); + Self::InternalError + } + SymCacheError::Parsing(e) => { + tracing::error!(error = %e, "failed to parse symcache"); + Self::InternalError + } + SymCacheError::Writing(e) => { + tracing::error!(error = %e, "failed to write symcache"); + Self::InternalError + } + SymCacheError::ObjectParsing(e) => { + tracing::error!(error = %e, "failed to parse object"); + Self::InternalError + } + SymCacheError::BcSymbolMapError(e) => { + tracing::error!(error = %e, "failed to handle auxiliary BCSymbolMap file"); + Self::InternalError + } + SymCacheError::Il2cppError(e) => { + tracing::error!(error = %e, "failed to handle auxiliary il2cpp line mapping file"); + Self::InternalError + } SymCacheError::Fetching(ref e) => Self::DownloadError(e.to_string()), SymCacheError::Malformed => Self::Malformed(String::new()), SymCacheError::Timeout => Self::Timeout(Duration::default()), From 7750ab5ecb476a5f930627add6f5048d2a99e5b2 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Fri, 2 Dec 2022 14:41:31 +0100 Subject: [PATCH 08/14] Don't pass candidates and features around unnecessarily --- .../src/services/ppdb_caches.rs | 55 +++++------------- .../src/services/symcaches/mod.rs | 56 +++++-------------- 2 files changed, 26 insertions(+), 85 deletions(-) diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index 83af7784c..5b518c4e5 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -107,8 +107,6 @@ impl From<&PortablePdbCacheError> for CacheEntry { pub struct PortablePdbCacheFile { data: ByteView<'static>, status: CacheStatus, - candidates: AllObjectCandidates, - features: ObjectFeatures, } impl PortablePdbCacheFile { @@ -126,16 +124,6 @@ impl PortablePdbCacheFile { CacheStatus::CacheSpecificError(_) => Err(PortablePdbCacheError::Malformed), } } - - /// Returns the list of DIFs which were searched for this ppdb cache. - pub fn candidates(&self) -> &AllObjectCandidates { - &self.candidates - } - - /// Returns the features of the object file this ppdb cache was constructed from. - pub fn features(&self) -> ObjectFeatures { - self.features - } } /// Information for fetching the symbols for this ppdb cache @@ -172,7 +160,7 @@ impl PortablePdbCacheActor { AllObjectCandidates, ObjectFeatures, ) { - let Ok(FoundObject { meta, candidates }) = self + let Ok(FoundObject { meta, mut candidates }) = self .objects .find(FindObject { filetypes: &[FileType::PortablePdb], @@ -192,22 +180,23 @@ impl PortablePdbCacheActor { .compute_memoized(FetchPortablePdbCacheInternal { request, objects_actor: self.objects.clone(), - object_meta: handle, - candidates: candidates.clone(), + object_meta: Arc::clone(&handle), }) .await { Ok(ppdb_cache_file) => { - let PortablePdbCacheFile { - status, - data, - candidates, - features, - .. - } = Arc::try_unwrap(ppdb_cache_file).unwrap_or_else(|arc| (*arc).clone()); + let PortablePdbCacheFile { status, data, .. } = + Arc::try_unwrap(ppdb_cache_file).unwrap_or_else(|arc| (*arc).clone()); + + candidates.set_debug( + handle.source_id(), + &handle.uri(), + ObjectUseInfo::from_derived_status(&status, handle.status()), + ); + let cache_entry = CacheEntry::from((status, data)); let mapped = cache_entry.try_map(parse_ppdb_cache_owned); - (mapped, candidates, features) + (mapped, candidates, handle.features()) } Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), } @@ -227,12 +216,6 @@ struct FetchPortablePdbCacheInternal { /// ObjectMeta handle of the original DIF object to fetch. object_meta: Arc, - - /// The object candidates from which [`FetchPortablePdbCacheInternal::object_meta`] was chosen. - /// - /// This needs to be returned back with the symcache result and is only being passed - /// through here as callers to the PortablePdbCacheActer want to have this info. - candidates: AllObjectCandidates, } /// Fetches the needed DIF objects and spawns symcache computation. @@ -314,19 +297,7 @@ impl CacheItemRequest for FetchPortablePdbCacheInternal { data: ByteView<'static>, _expiration: ExpirationTime, ) -> Self::Item { - let mut candidates = self.candidates.clone(); // yuk! - candidates.set_debug( - self.object_meta.source_id(), - &self.object_meta.uri(), - ObjectUseInfo::from_derived_status(&status, self.object_meta.status()), - ); - - PortablePdbCacheFile { - data, - status, - candidates, - features: self.object_meta.features(), - } + PortablePdbCacheFile { data, status } } } diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index 5170c2e13..a88529278 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -171,10 +171,8 @@ impl SymCacheActor { #[derive(Clone, Debug)] pub struct SymCacheFile { data: ByteView<'static>, - features: ObjectFeatures, status: CacheStatus, arch: Arch, - candidates: AllObjectCandidates, } impl SymCacheFile { @@ -195,16 +193,6 @@ impl SymCacheFile { pub fn arch(&self) -> Arch { self.arch } - - /// Returns the features of the object file this symcache was constructed from. - pub fn features(&self) -> ObjectFeatures { - self.features - } - - /// Returns the list of DIFs which were searched for this symcache. - pub fn candidates(&self) -> &AllObjectCandidates { - &self.candidates - } } #[derive(Clone, Debug)] @@ -220,12 +208,6 @@ struct FetchSymCacheInternal { /// ObjectMeta handle of the original DIF object to fetch. object_meta: Arc, - - /// The object candidates from which [`FetchSymCacheInternal::object_meta`] was chosen. - /// - /// This needs to be returned back with the symcache result and is only being passed - /// through here as callers to the SymCacheActer want to have this info. - candidates: AllObjectCandidates, } /// Fetches the needed DIF objects and spawns symcache computation. @@ -328,20 +310,7 @@ impl CacheItemRequest for FetchSymCacheInternal { .map(|cache| cache.arch()) .unwrap_or_default(); - let mut candidates = self.candidates.clone(); // yuk! - candidates.set_debug( - self.object_meta.source_id(), - &self.object_meta.uri(), - ObjectUseInfo::from_derived_status(&status, self.object_meta.status()), - ); - - SymCacheFile { - data, - features: self.object_meta.features(), - status, - arch, - candidates, - } + SymCacheFile { data, status, arch } } } @@ -363,7 +332,7 @@ impl SymCacheActor { AllObjectCandidates, ObjectFeatures, ) { - let Ok(FoundObject { meta, candidates }) = self + let Ok(FoundObject { meta, mut candidates }) = self .objects .find(FindObject { filetypes: FileType::from_object_type(request.object_type), @@ -426,22 +395,23 @@ impl SymCacheActor { request, objects_actor: self.objects.clone(), secondary_sources, - object_meta: handle, - candidates: candidates.clone(), + object_meta: Arc::clone(&handle), }) .await { Ok(symcache_file) => { - let SymCacheFile { - status, - data, - candidates, - features, - .. - } = Arc::try_unwrap(symcache_file).unwrap_or_else(|arc| (*arc).clone()); + let SymCacheFile { status, data, .. } = + Arc::try_unwrap(symcache_file).unwrap_or_else(|arc| (*arc).clone()); + + candidates.set_debug( + handle.source_id(), + &handle.uri(), + ObjectUseInfo::from_derived_status(&status, handle.status()), + ); + let cache_entry = CacheEntry::from((status, data)); let mapped = cache_entry.try_map(parse_symcache_owned); - (mapped, candidates, features) + (mapped, candidates, handle.features()) } Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), } From ee99da60545bbfd3d6aefa729677b07a0779ffa8 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Fri, 2 Dec 2022 18:55:28 +0100 Subject: [PATCH 09/14] Further simplify ppdb and symcache actors --- crates/symbolicator-service/src/cache.rs | 14 +++ .../src/services/ppdb_caches.rs | 85 ++++++++------- .../services/symbolication/module_lookup.rs | 26 +++-- .../src/services/symcaches/mod.rs | 102 ++++++++---------- 4 files changed, 120 insertions(+), 107 deletions(-) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index 683fdbcca..36cab3083 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -234,6 +234,20 @@ impl CacheEntry { pub fn is_not_found(&self) -> bool { matches!(self, Self::NotFound) } + + pub fn as_cache_status(&self) -> CacheStatus { + match self { + Self::AllGood(_) => CacheStatus::Positive, + Self::NotFound => CacheStatus::Negative, + Self::DownloadError(s) => CacheStatus::CacheSpecificError(s.clone()), + Self::PermissionDenied(_) => { + CacheStatus::CacheSpecificError("missing permissions for file".into()) + } + Self::Timeout(_) => CacheStatus::CacheSpecificError("download was cancelled".into()), + Self::Malformed(s) => CacheStatus::Malformed(s.clone()), + Self::InternalError => CacheStatus::CacheSpecificError("internal error".into()), + } + } } impl CacheEntry> { diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index 5b518c4e5..9e9a9f56b 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -103,29 +103,6 @@ impl From<&PortablePdbCacheError> for CacheEntry { } } -#[derive(Debug, Clone)] -pub struct PortablePdbCacheFile { - data: ByteView<'static>, - status: CacheStatus, -} - -impl PortablePdbCacheFile { - pub fn parse( - &self, - ) -> Result>, PortablePdbCacheError> { - match &self.status { - CacheStatus::Positive => Ok(Some( - PortablePdbCache::parse(&self.data).map_err(PortablePdbCacheError::Parsing)?, - )), - CacheStatus::Negative => Ok(None), - CacheStatus::Malformed(_) => Err(PortablePdbCacheError::Malformed), - // If the cache entry is for a cache specific error, it must be - // from a previous symcache conversion attempt. - CacheStatus::CacheSpecificError(_) => Err(PortablePdbCacheError::Malformed), - } - } -} - /// Information for fetching the symbols for this ppdb cache #[derive(Debug, Clone)] pub struct FetchPortablePdbCache { @@ -134,6 +111,23 @@ pub struct FetchPortablePdbCache { pub scope: Scope, } +#[derive(Debug, Clone)] +pub struct FetchPortablePdbCacheResponse { + pub cache: CacheEntry, + pub candidates: AllObjectCandidates, + pub features: ObjectFeatures, +} + +impl Default for FetchPortablePdbCacheResponse { + fn default() -> Self { + Self { + cache: CacheEntry::InternalError, + candidates: Default::default(), + features: Default::default(), + } + } +} + #[derive(Clone, Debug)] pub struct PortablePdbCacheActor { ppdb_caches: Arc>, @@ -152,14 +146,7 @@ impl PortablePdbCacheActor { } } - pub async fn fetch( - &self, - request: FetchPortablePdbCache, - ) -> ( - CacheEntry, - AllObjectCandidates, - ObjectFeatures, - ) { + pub async fn fetch(&self, request: FetchPortablePdbCache) -> FetchPortablePdbCacheResponse { let Ok(FoundObject { meta, mut candidates }) = self .objects .find(FindObject { @@ -170,7 +157,7 @@ impl PortablePdbCacheActor { purpose: ObjectPurpose::Debug, }) .await else { - return (CacheEntry::InternalError, AllObjectCandidates::default(), ObjectFeatures::default()) + return FetchPortablePdbCacheResponse::default() }; match meta { @@ -185,23 +172,34 @@ impl PortablePdbCacheActor { .await { Ok(ppdb_cache_file) => { - let PortablePdbCacheFile { status, data, .. } = - Arc::try_unwrap(ppdb_cache_file).unwrap_or_else(|arc| (*arc).clone()); - candidates.set_debug( handle.source_id(), &handle.uri(), - ObjectUseInfo::from_derived_status(&status, handle.status()), + ObjectUseInfo::from_derived_status( + &ppdb_cache_file.as_cache_status(), + handle.status(), + ), ); - let cache_entry = CacheEntry::from((status, data)); - let mapped = cache_entry.try_map(parse_ppdb_cache_owned); - (mapped, candidates, handle.features()) + FetchPortablePdbCacheResponse { + cache: Arc::try_unwrap(ppdb_cache_file) + .unwrap_or_else(|arc| (*arc).clone()), + candidates, + features: handle.features(), + } } - Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), + Err(e) => FetchPortablePdbCacheResponse { + cache: e.as_ref().into(), + candidates, + features: ObjectFeatures::default(), + }, } } - None => (CacheEntry::NotFound, candidates, ObjectFeatures::default()), + None => FetchPortablePdbCacheResponse { + cache: CacheEntry::NotFound, + candidates, + features: ObjectFeatures::default(), + }, } } } @@ -259,7 +257,7 @@ async fn fetch_difs_and_compute_ppdb_cache( } impl CacheItemRequest for FetchPortablePdbCacheInternal { - type Item = PortablePdbCacheFile; + type Item = CacheEntry; type Error = PortablePdbCacheError; const VERSIONS: CacheVersions = PPDB_CACHE_VERSIONS; @@ -297,7 +295,8 @@ impl CacheItemRequest for FetchPortablePdbCacheInternal { data: ByteView<'static>, _expiration: ExpirationTime, ) -> Self::Item { - PortablePdbCacheFile { data, status } + let cache_entry = CacheEntry::from((status, data)); + cache_entry.try_map(parse_ppdb_cache_owned) } } diff --git a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs index 064aef084..683616c68 100644 --- a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs +++ b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs @@ -12,9 +12,12 @@ use symbolicator_sources::{FileType, ObjectType, SourceConfig}; use crate::cache::CacheEntry; use crate::services::objects::{FindObject, FoundObject, ObjectPurpose, ObjectsActor}; use crate::services::ppdb_caches::{ - FetchPortablePdbCache, OwnedPortablePdbCache, PortablePdbCacheActor, PortablePdbCacheError, + FetchPortablePdbCache, FetchPortablePdbCacheResponse, OwnedPortablePdbCache, + PortablePdbCacheActor, PortablePdbCacheError, +}; +use crate::services::symcaches::{ + FetchSymCache, FetchSymCacheResponse, OwnedSymCache, SymCacheActor, SymCacheError, }; -use crate::services::symcaches::{FetchSymCache, OwnedSymCache, SymCacheActor, SymCacheError}; use crate::types::{ AllObjectCandidates, CompleteObjectInfo, CompleteStacktrace, ObjectFeatures, ObjectFileStatus, RawStacktrace, Scope, @@ -206,13 +209,17 @@ impl ModuleLookup { sources, scope, }; - let (ppdb_cache_entry, candidates, features) = - ppdb_cache_actor.fetch(request).await; + + let FetchPortablePdbCacheResponse { + cache, + candidates, + features, + } = ppdb_cache_actor.fetch(request).await; ( idx, CacheFile { - file: ppdb_cache_entry.map(CacheFileEntry::PortablePdbCache), + file: cache.map(CacheFileEntry::PortablePdbCache), candidates, features, }, @@ -226,13 +233,16 @@ impl ModuleLookup { scope, }; - let (symcache_entry, candidates, features) = - symcache_actor.fetch(request).await; + let FetchSymCacheResponse { + cache, + candidates, + features, + } = symcache_actor.fetch(request).await; ( idx, CacheFile { - file: symcache_entry.map(CacheFileEntry::SymCache), + file: cache.map(CacheFileEntry::SymCache), candidates, features, }, diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index a88529278..e26c325e7 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -8,7 +8,7 @@ use anyhow::Error; use futures::future::BoxFuture; use thiserror::Error; -use symbolic::common::{Arch, ByteView, SelfCell}; +use symbolic::common::{ByteView, SelfCell}; use symbolic::symcache::{self, SymCache, SymCacheConverter}; use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig}; @@ -168,33 +168,6 @@ impl SymCacheActor { } } -#[derive(Clone, Debug)] -pub struct SymCacheFile { - data: ByteView<'static>, - status: CacheStatus, - arch: Arch, -} - -impl SymCacheFile { - pub fn parse(&self) -> Result>, SymCacheError> { - match &self.status { - CacheStatus::Positive => Ok(Some( - SymCache::parse(&self.data).map_err(SymCacheError::Parsing)?, - )), - CacheStatus::Negative => Ok(None), - CacheStatus::Malformed(_) => Err(SymCacheError::Malformed), - // If the cache entry is for a cache specific error, it must be - // from a previous symcache conversion attempt. - CacheStatus::CacheSpecificError(_) => Err(SymCacheError::Malformed), - } - } - - /// Returns the architecture of this symcache. - pub fn arch(&self) -> Arch { - self.arch - } -} - #[derive(Clone, Debug)] struct FetchSymCacheInternal { /// The external request, as passed into [`SymCacheActor::fetch`]. @@ -258,7 +231,7 @@ async fn fetch_difs_and_compute_symcache( } impl CacheItemRequest for FetchSymCacheInternal { - type Item = SymCacheFile; + type Item = CacheEntry; type Error = SymCacheError; const VERSIONS: CacheVersions = SYMCACHE_VERSIONS; @@ -305,12 +278,8 @@ impl CacheItemRequest for FetchSymCacheInternal { data: ByteView<'static>, _expiration: ExpirationTime, ) -> Self::Item { - // TODO: Figure out if this double-parsing could be avoided - let arch = SymCache::parse(&data) - .map(|cache| cache.arch()) - .unwrap_or_default(); - - SymCacheFile { data, status, arch } + let cache_entry = CacheEntry::from((status, data)); + cache_entry.try_map(parse_symcache_owned) } } @@ -323,15 +292,25 @@ pub struct FetchSymCache { pub scope: Scope, } +#[derive(Debug, Clone)] +pub struct FetchSymCacheResponse { + pub cache: CacheEntry, + pub candidates: AllObjectCandidates, + pub features: ObjectFeatures, +} + +impl Default for FetchSymCacheResponse { + fn default() -> Self { + Self { + cache: CacheEntry::InternalError, + candidates: Default::default(), + features: Default::default(), + } + } +} + impl SymCacheActor { - pub async fn fetch( - &self, - request: FetchSymCache, - ) -> ( - CacheEntry, - AllObjectCandidates, - ObjectFeatures, - ) { + pub async fn fetch(&self, request: FetchSymCache) -> FetchSymCacheResponse { let Ok(FoundObject { meta, mut candidates }) = self .objects .find(FindObject { @@ -342,7 +321,7 @@ impl SymCacheActor { purpose: ObjectPurpose::Debug, }) .await else { - return (CacheEntry::InternalError, AllObjectCandidates::default(), ObjectFeatures::default()) + return FetchSymCacheResponse::default(); }; match meta { @@ -400,23 +379,34 @@ impl SymCacheActor { .await { Ok(symcache_file) => { - let SymCacheFile { status, data, .. } = - Arc::try_unwrap(symcache_file).unwrap_or_else(|arc| (*arc).clone()); - candidates.set_debug( handle.source_id(), &handle.uri(), - ObjectUseInfo::from_derived_status(&status, handle.status()), + ObjectUseInfo::from_derived_status( + &symcache_file.as_cache_status(), + handle.status(), + ), ); - let cache_entry = CacheEntry::from((status, data)); - let mapped = cache_entry.try_map(parse_symcache_owned); - (mapped, candidates, handle.features()) + FetchSymCacheResponse { + cache: Arc::try_unwrap(symcache_file) + .unwrap_or_else(|arc| (*arc).clone()), + candidates, + features: handle.features(), + } } - Err(e) => (e.as_ref().into(), candidates, ObjectFeatures::default()), + Err(e) => FetchSymCacheResponse { + cache: e.as_ref().into(), + candidates, + features: ObjectFeatures::default(), + }, } } - None => (CacheEntry::NotFound, candidates, ObjectFeatures::default()), + None => FetchSymCacheResponse { + cache: CacheEntry::NotFound, + candidates, + features: ObjectFeatures::default(), + }, } } } @@ -606,7 +596,7 @@ mod tests { let owned_symcache = symcache_actor .fetch(fetch_symcache.clone()) .await - .0 + .cache .all_good() .unwrap(); @@ -637,7 +627,7 @@ mod tests { let owned_symcache = symcache_actor .fetch(fetch_symcache.clone()) .await - .0 + .cache .all_good() .unwrap(); @@ -657,7 +647,7 @@ mod tests { let owned_symcache = symcache_actor .fetch(fetch_symcache.clone()) .await - .0 + .cache .all_good() .unwrap(); From fb17a222632975b09eb3e2d72ba5576983ad7c34 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Fri, 2 Dec 2022 19:37:38 +0100 Subject: [PATCH 10/14] Docs --- crates/symbolicator-service/src/cache.rs | 61 +++++++++++++++++++++--- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index 36cab3083..78233efa3 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -120,8 +120,14 @@ impl CacheStatus { } } -/// A cache entry that either contains an object of type `T` -/// or one of several error conditions. +/// A cache entry that represents the result of fetching an object of type `T` from +/// a remote location. +/// +/// # Reading from a file +/// +/// A `CacheEntry` can be read from a [`ByteView`] using +/// [`from_bytes`](CacheEntry::from_bytes). That `CacheEntry` can then be converted +/// using [`map`](CacheEntry::map) or [`try_map`](CacheEntry::try_map). #[derive(Debug, Clone, PartialEq, Eq)] pub enum CacheEntry { /// A valid cache entry containing an instance of `T`. @@ -134,7 +140,7 @@ pub enum CacheEntry { /// The attached string contains the remote source's response. PermissionDenied(String), // => whatever the server returned /// The object could not be fetched from the remote source due to a timeout. - Timeout(Duration), // => should we return the duration after which we timed out? + Timeout(Duration), /// The object could not be fetched from the remote source due to another problem, /// like connection loss, DNS resolution, or a 5xx server response. /// @@ -146,6 +152,8 @@ pub enum CacheEntry { /// during symcache conversion Malformed(String), /// An unexpected error in symbolicator itself. + /// + /// This variant is not intended to be persisted to or read from disk. InternalError, } @@ -155,38 +163,60 @@ impl CacheEntry { const DOWNLOAD_ERROR_MARKER: &[u8] = b"downloaderror"; const MALFORMED_MARKER: &[u8] = b"malformed"; + /// Writes error markers and details to a file. + /// + /// * If `self` is [`AllGood`](CacheEntry::AllGood), this merely seeks to + /// the end of the file. + /// * If `self` is [`InternalError`](CacheEntry::InternalError), it does nothing. + /// * If `self` is [`NotFound`](CacheEntry::NotFound), it empties the file. + /// * In all other cases, it writes the corresponding marker, followed by the error + /// details, and truncates the file. pub async fn write(&self, file: &mut File) -> Result<(), io::Error> { - file.rewind().await?; match self { - CacheEntry::AllGood(_entry) => {} + CacheEntry::AllGood(_) => { + file.seek(SeekFrom::End(0)).await?; + } CacheEntry::NotFound => { + file.rewind().await?; file.set_len(0).await?; } CacheEntry::PermissionDenied(details) => { + file.rewind().await?; file.write_all(Self::PERMISSION_DENIED_MARKER).await?; file.write_all(details.as_bytes()).await?; + let new_len = file.stream_position().await?; + file.set_len(new_len).await?; } CacheEntry::Timeout(duration) => { + file.rewind().await?; file.write_all(Self::TIMEOUT_MARKER).await?; file.write_all(format_duration(*duration).to_string().as_bytes()) .await?; + let new_len = file.stream_position().await?; + file.set_len(new_len).await?; } CacheEntry::DownloadError(details) => { + file.rewind().await?; file.write_all(Self::DOWNLOAD_ERROR_MARKER).await?; file.write_all(details.as_bytes()).await?; + let new_len = file.stream_position().await?; + file.set_len(new_len).await?; } CacheEntry::Malformed(details) => { + file.rewind().await?; file.write_all(Self::MALFORMED_MARKER).await?; file.write_all(details.as_bytes()).await?; + let new_len = file.stream_position().await?; + file.set_len(new_len).await?; } CacheEntry::InternalError => {} } - let new_len = file.stream_position().await?; - file.set_len(new_len).await?; Ok(()) } + /// Maps a `CacheEntry` to `CacheEntry` by applying a function to a contained + /// [`AllGood`](CacheEntry::AllGood) value, leaving all other variants alone. pub fn map(self, f: F) -> CacheEntry where F: FnOnce(T) -> U, @@ -202,6 +232,11 @@ impl CacheEntry { } } + /// Maps a `CacheEntry` to `CacheEntry` by applying a fallible function to a contained + /// [`AllGood`](CacheEntry::AllGood) value, leaving all other variants alone. + /// + /// If the function returns an error, that error will be logged and an + /// [`InternalError`](CacheEntry::InternalError) will be returned. pub fn try_map(self, f: F) -> CacheEntry where E: std::error::Error, @@ -224,6 +259,8 @@ impl CacheEntry { } } + /// Returns `Some(x)` if `self` is [`AllGood(x)`](CacheEntry::AllGood), otherwise + /// returns `None`. pub fn all_good(self) -> Option { match self { Self::AllGood(entry) => Some(entry), @@ -231,10 +268,14 @@ impl CacheEntry { } } + /// Returns `true` if `self` is [`NotFound`](CacheEntry::NotFound). pub fn is_not_found(&self) -> bool { matches!(self, Self::NotFound) } + /// Returns the [`CacheStatus`] corresponding to this `CacheEntry`. + /// + /// Note that this method allocates strings for some of the variants. pub fn as_cache_status(&self) -> CacheStatus { match self { Self::AllGood(_) => CacheStatus::Positive, @@ -251,6 +292,12 @@ impl CacheEntry { } impl CacheEntry> { + /// Parses a `CacheEntry` from a [`ByteView`](ByteView). + /// + /// * If the file starts with an error marker, the corresponding error variant will be returned. + /// * If the file is empty, [`NotFound`](CacheEntry::NotFound) will be returned. + /// * In any other case, an [`AllGood`](CacheEntry::AllGood) containing the `ByteView` itself will be + /// returned. pub fn from_bytes(bytes: ByteView<'static>) -> Self { if let Some(raw_message) = bytes.strip_prefix(Self::PERMISSION_DENIED_MARKER) { let err_msg = String::from_utf8_lossy(raw_message); From 1fe4d72a335924f4ebe067d1d703fc1cfedf142a Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Mon, 5 Dec 2022 15:04:27 +0100 Subject: [PATCH 11/14] Replace CacheEntry enum with Result --- crates/symbolicator-service/src/cache.rs | 220 +++++++----------- .../src/services/ppdb_caches.rs | 28 ++- .../services/symbolication/module_lookup.rs | 20 +- .../services/symbolication/symbolication.rs | 8 +- .../src/services/symcaches/mod.rs | 34 +-- 5 files changed, 140 insertions(+), 170 deletions(-) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index 78233efa3..e409e06db 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -120,25 +120,19 @@ impl CacheStatus { } } -/// A cache entry that represents the result of fetching an object of type `T` from -/// a remote location. +/// An error that happens when fetching an object from a remote location. /// -/// # Reading from a file -/// -/// A `CacheEntry` can be read from a [`ByteView`] using -/// [`from_bytes`](CacheEntry::from_bytes). That `CacheEntry` can then be converted -/// using [`map`](CacheEntry::map) or [`try_map`](CacheEntry::try_map). +/// This error enum is intended for persisting in caches, except for the +/// [`InternalError`](Self::InternalError) variant. #[derive(Debug, Clone, PartialEq, Eq)] -pub enum CacheEntry { - /// A valid cache entry containing an instance of `T`. - AllGood(T), - /// The object was not found at the remote location. +pub enum CacheError { + /// The object was not found at the remote source. NotFound, /// The object could not be fetched from the remote source due to missing /// permissions. /// /// The attached string contains the remote source's response. - PermissionDenied(String), // => whatever the server returned + PermissionDenied(String), /// The object could not be fetched from the remote source due to a timeout. Timeout(Duration), /// The object could not be fetched from the remote source due to another problem, @@ -153,11 +147,11 @@ pub enum CacheEntry { Malformed(String), /// An unexpected error in symbolicator itself. /// - /// This variant is not intended to be persisted to or read from disk. + /// This variant is not intended to be persisted to or read from caches. InternalError, } -impl CacheEntry { +impl CacheError { const PERMISSION_DENIED_MARKER: &[u8] = b"permissiondenied"; const TIMEOUT_MARKER: &[u8] = b"timeout"; const DOWNLOAD_ERROR_MARKER: &[u8] = b"downloaderror"; @@ -165,29 +159,24 @@ impl CacheEntry { /// Writes error markers and details to a file. /// - /// * If `self` is [`AllGood`](CacheEntry::AllGood), this merely seeks to - /// the end of the file. - /// * If `self` is [`InternalError`](CacheEntry::InternalError), it does nothing. - /// * If `self` is [`NotFound`](CacheEntry::NotFound), it empties the file. + /// * If `self` is [`InternalError`](Self::InternalError), it does nothing. + /// * If `self` is [`NotFound`](Self::NotFound), it empties the file. /// * In all other cases, it writes the corresponding marker, followed by the error /// details, and truncates the file. pub async fn write(&self, file: &mut File) -> Result<(), io::Error> { match self { - CacheEntry::AllGood(_) => { - file.seek(SeekFrom::End(0)).await?; - } - CacheEntry::NotFound => { + Self::NotFound => { file.rewind().await?; file.set_len(0).await?; } - CacheEntry::PermissionDenied(details) => { + Self::PermissionDenied(details) => { file.rewind().await?; file.write_all(Self::PERMISSION_DENIED_MARKER).await?; file.write_all(details.as_bytes()).await?; let new_len = file.stream_position().await?; file.set_len(new_len).await?; } - CacheEntry::Timeout(duration) => { + Self::Timeout(duration) => { file.rewind().await?; file.write_all(Self::TIMEOUT_MARKER).await?; file.write_all(format_duration(*duration).to_string().as_bytes()) @@ -195,90 +184,62 @@ impl CacheEntry { let new_len = file.stream_position().await?; file.set_len(new_len).await?; } - CacheEntry::DownloadError(details) => { + Self::DownloadError(details) => { file.rewind().await?; file.write_all(Self::DOWNLOAD_ERROR_MARKER).await?; file.write_all(details.as_bytes()).await?; let new_len = file.stream_position().await?; file.set_len(new_len).await?; } - CacheEntry::Malformed(details) => { + Self::Malformed(details) => { file.rewind().await?; file.write_all(Self::MALFORMED_MARKER).await?; file.write_all(details.as_bytes()).await?; let new_len = file.stream_position().await?; file.set_len(new_len).await?; } - CacheEntry::InternalError => {} + Self::InternalError => {} } Ok(()) } - /// Maps a `CacheEntry` to `CacheEntry` by applying a function to a contained - /// [`AllGood`](CacheEntry::AllGood) value, leaving all other variants alone. - pub fn map(self, f: F) -> CacheEntry - where - F: FnOnce(T) -> U, - { - match self { - Self::AllGood(entry) => CacheEntry::AllGood(f(entry)), - Self::NotFound => CacheEntry::NotFound, - Self::PermissionDenied(details) => CacheEntry::PermissionDenied(details), - Self::Timeout(duration) => CacheEntry::Timeout(duration), - Self::DownloadError(details) => CacheEntry::DownloadError(details), - Self::Malformed(details) => CacheEntry::Malformed(details), - Self::InternalError => CacheEntry::InternalError, - } - } - - /// Maps a `CacheEntry` to `CacheEntry` by applying a fallible function to a contained - /// [`AllGood`](CacheEntry::AllGood) value, leaving all other variants alone. + /// Parses a `CacheError` from a byte slice. /// - /// If the function returns an error, that error will be logged and an - /// [`InternalError`](CacheEntry::InternalError) will be returned. - pub fn try_map(self, f: F) -> CacheEntry - where - E: std::error::Error, - F: FnOnce(T) -> Result, - { - match self { - Self::AllGood(entry) => match f(entry) { - Ok(new_entry) => CacheEntry::AllGood(new_entry), + /// * If the slice starts with an error marker, the corresponding error variant will be returned. + /// * If the slice is empty, [`NotFound`](CacheEntry::NotFound) will be returned. + /// * Otherwise `None` is returned. + fn from_bytes(bytes: &[u8]) -> Option { + if let Some(raw_message) = bytes.strip_prefix(CacheError::PERMISSION_DENIED_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Some(Self::PermissionDenied(err_msg.into_owned())) + } else if let Some(raw_duration) = bytes.strip_prefix(Self::TIMEOUT_MARKER) { + let raw_duration = String::from_utf8_lossy(raw_duration); + match parse_duration(&raw_duration) { + Ok(duration) => Some(Self::Timeout(duration)), Err(e) => { - tracing::error!(error = %e); - CacheEntry::InternalError + tracing::error!(error = %e, "Failed to read timeout duration"); + Some(Self::InternalError) } - }, - Self::NotFound => CacheEntry::NotFound, - Self::PermissionDenied(details) => CacheEntry::PermissionDenied(details), - Self::Timeout(duration) => CacheEntry::Timeout(duration), - Self::DownloadError(details) => CacheEntry::DownloadError(details), - Self::Malformed(details) => CacheEntry::Malformed(details), - Self::InternalError => CacheEntry::InternalError, - } - } - - /// Returns `Some(x)` if `self` is [`AllGood(x)`](CacheEntry::AllGood), otherwise - /// returns `None`. - pub fn all_good(self) -> Option { - match self { - Self::AllGood(entry) => Some(entry), - _ => None, + } + } else if let Some(raw_message) = bytes.strip_prefix(Self::DOWNLOAD_ERROR_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Some(Self::DownloadError(err_msg.into_owned())) + } else if let Some(raw_message) = bytes.strip_prefix(Self::MALFORMED_MARKER) { + let err_msg = String::from_utf8_lossy(raw_message); + Some(Self::Malformed(err_msg.into_owned())) + } else if bytes.is_empty() { + Some(Self::NotFound) + } else { + None } } - /// Returns `true` if `self` is [`NotFound`](CacheEntry::NotFound). - pub fn is_not_found(&self) -> bool { - matches!(self, Self::NotFound) - } - - /// Returns the [`CacheStatus`] corresponding to this `CacheEntry`. + /// Returns the [`CacheStatus`] corresponding to this `CacheError`. /// /// Note that this method allocates strings for some of the variants. pub fn as_cache_status(&self) -> CacheStatus { match self { - Self::AllGood(_) => CacheStatus::Positive, Self::NotFound => CacheStatus::Negative, Self::DownloadError(s) => CacheStatus::CacheSpecificError(s.clone()), Self::PermissionDenied(_) => { @@ -291,56 +252,43 @@ impl CacheEntry { } } -impl CacheEntry> { - /// Parses a `CacheEntry` from a [`ByteView`](ByteView). - /// - /// * If the file starts with an error marker, the corresponding error variant will be returned. - /// * If the file is empty, [`NotFound`](CacheEntry::NotFound) will be returned. - /// * In any other case, an [`AllGood`](CacheEntry::AllGood) containing the `ByteView` itself will be - /// returned. - pub fn from_bytes(bytes: ByteView<'static>) -> Self { - if let Some(raw_message) = bytes.strip_prefix(Self::PERMISSION_DENIED_MARKER) { - let err_msg = String::from_utf8_lossy(raw_message); - Self::PermissionDenied(err_msg.into_owned()) - } else if let Some(raw_duration) = bytes.strip_prefix(Self::TIMEOUT_MARKER) { - let raw_duration = String::from_utf8_lossy(raw_duration); - match parse_duration(&raw_duration) { - Ok(duration) => Self::Timeout(duration), - Err(e) => { - tracing::error!(error = %e, "Failed to read timeout duration"); - Self::InternalError - } +/// An entry in a cache, containing either `Ok(T)` or an error denoting the reason why an +/// object could not be fetched or is otherwise unusable. +pub type CacheEntry = Result; + +/// Parses a `CacheEntry` from a [`ByteView`](ByteView). +pub fn cache_entry_from_bytes(bytes: ByteView<'static>) -> CacheEntry> { + CacheError::from_bytes(&bytes).map(Err).unwrap_or(Ok(bytes)) +} + +/// Transforms a `([CacheStatus], [ByteView])` pair into a [`CacheEntry`]. +pub fn cache_entry_from_cache_status( + status: CacheStatus, + content: ByteView<'static>, +) -> CacheEntry> { + match status { + CacheStatus::Positive => Ok(content), + CacheStatus::Negative => Err(CacheError::NotFound), + CacheStatus::Malformed(s) => Err(CacheError::Malformed(s)), + CacheStatus::CacheSpecificError(message) => { + if let Some(details) = message.strip_prefix("missing permissions for file") { + Err(CacheError::PermissionDenied(details.to_string())) + } else if message == "download was cancelled" { + Err(CacheError::Timeout(Duration::from_secs(0))) + } else { + Err(CacheError::DownloadError(message)) } - } else if let Some(raw_message) = bytes.strip_prefix(Self::DOWNLOAD_ERROR_MARKER) { - let err_msg = String::from_utf8_lossy(raw_message); - Self::DownloadError(err_msg.into_owned()) - } else if let Some(raw_message) = bytes.strip_prefix(Self::MALFORMED_MARKER) { - let err_msg = String::from_utf8_lossy(raw_message); - Self::Malformed(err_msg.into_owned()) - } else if bytes.is_empty() { - Self::NotFound - } else { - Self::AllGood(bytes) } } } -impl From<(CacheStatus, ByteView<'static>)> for CacheEntry> { - fn from((status, content): (CacheStatus, ByteView<'static>)) -> Self { - match status { - CacheStatus::Positive => Self::AllGood(content), - CacheStatus::Negative => Self::NotFound, - CacheStatus::Malformed(s) => Self::Malformed(s), - CacheStatus::CacheSpecificError(message) => { - if let Some(details) = message.strip_prefix("missing permissions for file") { - Self::PermissionDenied(details.to_string()) - } else if message == "download was cancelled" { - Self::Timeout(Duration::from_secs(0)) - } else { - Self::DownloadError(message) - } - } - } +/// Returns the [`CacheStatus`] corresponding to the given `CacheEntry`. +/// +/// Note that this function allocates strings for some of the variants. +pub fn cache_entry_as_cache_status(entry: &CacheEntry) -> CacheStatus { + match entry { + Ok(_) => CacheStatus::Positive, + Err(e) => e.as_cache_status(), } } @@ -1828,47 +1776,53 @@ mod tests { #[test] fn test_cache_entry() { fn read_cache_entry(bytes: &'static [u8]) -> CacheEntry { - CacheEntry::from_bytes(ByteView::from_slice(bytes)) + cache_entry_from_bytes(ByteView::from_slice(bytes)) .map(|bv| String::from_utf8_lossy(bv.as_slice()).into_owned()) } let not_found = b""; - assert_eq!(read_cache_entry(not_found), CacheEntry::NotFound); + assert_eq!(read_cache_entry(not_found), Err(CacheError::NotFound)); let malformed = b"malformedDoesn't look like anything to me"; assert_eq!( read_cache_entry(malformed), - CacheEntry::Malformed("Doesn't look like anything to me".into()) + Err(CacheError::Malformed( + "Doesn't look like anything to me".into() + )) ); let timeout = b"timeout4m33s"; assert_eq!( read_cache_entry(timeout), - CacheEntry::Timeout(Duration::from_secs(273)) + Err(CacheError::Timeout(Duration::from_secs(273))) ); let download_error = b"downloaderrorSomeone unplugged the internet"; assert_eq!( read_cache_entry(download_error), - CacheEntry::DownloadError("Someone unplugged the internet".into()) + Err(CacheError::DownloadError( + "Someone unplugged the internet".into() + )) ); let permission_denied = b"permissiondeniedI'm sorry Dave, I'm afraid I can't do that"; assert_eq!( read_cache_entry(permission_denied), - CacheEntry::PermissionDenied("I'm sorry Dave, I'm afraid I can't do that".into()) + Err(CacheError::PermissionDenied( + "I'm sorry Dave, I'm afraid I can't do that".into() + )) ); let all_good = b"Not any of the error cases"; assert_eq!( read_cache_entry(all_good), - CacheEntry::AllGood("Not any of the error cases".into()) + Ok("Not any of the error cases".into()) ); } } diff --git a/crates/symbolicator-service/src/services/ppdb_caches.rs b/crates/symbolicator-service/src/services/ppdb_caches.rs index 9e9a9f56b..38d399500 100644 --- a/crates/symbolicator-service/src/services/ppdb_caches.rs +++ b/crates/symbolicator-service/src/services/ppdb_caches.rs @@ -12,7 +12,10 @@ use symbolic::debuginfo::Object; use symbolic::ppdb::{PortablePdbCache, PortablePdbCacheConverter}; use symbolicator_sources::{FileType, ObjectId, SourceConfig}; -use crate::cache::{Cache, CacheEntry, CacheStatus, ExpirationTime}; +use crate::cache::{ + cache_entry_as_cache_status, cache_entry_from_cache_status, Cache, CacheEntry, CacheError, + CacheStatus, ExpirationTime, +}; use crate::services::objects::ObjectError; use crate::types::{AllObjectCandidates, ObjectFeatures, ObjectUseInfo, Scope}; use crate::utils::futures::{m, measure}; @@ -48,8 +51,13 @@ pub type OwnedPortablePdbCache = SelfCell, PortablePdbCache<'s fn parse_ppdb_cache_owned( byteview: ByteView<'static>, -) -> Result { - SelfCell::try_new(byteview, |p| unsafe { PortablePdbCache::parse(&*p) }) +) -> Result { + SelfCell::try_new(byteview, |p| unsafe { + PortablePdbCache::parse(&*p).map_err(|e| { + tracing::error!(error = %e); + CacheError::InternalError + }) + }) } /// Errors happening while generating a symcache. @@ -77,7 +85,7 @@ pub enum PortablePdbCacheError { Timeout, } -impl From<&PortablePdbCacheError> for CacheEntry { +impl From<&PortablePdbCacheError> for CacheError { fn from(error: &PortablePdbCacheError) -> Self { match error { PortablePdbCacheError::Io(e) => { @@ -121,7 +129,7 @@ pub struct FetchPortablePdbCacheResponse { impl Default for FetchPortablePdbCacheResponse { fn default() -> Self { Self { - cache: CacheEntry::InternalError, + cache: Err(CacheError::InternalError), candidates: Default::default(), features: Default::default(), } @@ -176,7 +184,7 @@ impl PortablePdbCacheActor { handle.source_id(), &handle.uri(), ObjectUseInfo::from_derived_status( - &ppdb_cache_file.as_cache_status(), + &cache_entry_as_cache_status(&ppdb_cache_file), handle.status(), ), ); @@ -189,14 +197,14 @@ impl PortablePdbCacheActor { } } Err(e) => FetchPortablePdbCacheResponse { - cache: e.as_ref().into(), + cache: Err(e.as_ref().into()), candidates, features: ObjectFeatures::default(), }, } } None => FetchPortablePdbCacheResponse { - cache: CacheEntry::NotFound, + cache: Err(CacheError::NotFound), candidates, features: ObjectFeatures::default(), }, @@ -295,8 +303,8 @@ impl CacheItemRequest for FetchPortablePdbCacheInternal { data: ByteView<'static>, _expiration: ExpirationTime, ) -> Self::Item { - let cache_entry = CacheEntry::from((status, data)); - cache_entry.try_map(parse_ppdb_cache_owned) + let cache_entry = cache_entry_from_cache_status(status, data); + cache_entry.and_then(parse_ppdb_cache_owned) } } diff --git a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs index 683616c68..a466b8718 100644 --- a/crates/symbolicator-service/src/services/symbolication/module_lookup.rs +++ b/crates/symbolicator-service/src/services/symbolication/module_lookup.rs @@ -9,7 +9,7 @@ use symbolic::common::{ByteView, SelfCell}; use symbolic::debuginfo::{Object, ObjectDebugSession}; use symbolicator_sources::{FileType, ObjectType, SourceConfig}; -use crate::cache::CacheEntry; +use crate::cache::{CacheEntry, CacheError}; use crate::services::objects::{FindObject, FoundObject, ObjectPurpose, ObjectsActor}; use crate::services::ppdb_caches::{ FetchPortablePdbCache, FetchPortablePdbCacheResponse, OwnedPortablePdbCache, @@ -28,14 +28,14 @@ use super::object_id_from_object_info; fn object_file_status_from_cache_entry(cache_entry: &CacheEntry) -> ObjectFileStatus { match cache_entry { - CacheEntry::AllGood(_) => ObjectFileStatus::Found, - CacheEntry::NotFound => ObjectFileStatus::Missing, - CacheEntry::PermissionDenied(_) | CacheEntry::DownloadError(_) => { + Ok(_) => ObjectFileStatus::Found, + Err(CacheError::NotFound) => ObjectFileStatus::Missing, + Err(CacheError::PermissionDenied(_) | CacheError::DownloadError(_)) => { ObjectFileStatus::FetchingFailed } - CacheEntry::Timeout(_) => ObjectFileStatus::Timeout, - CacheEntry::Malformed(_) => ObjectFileStatus::Malformed, - CacheEntry::InternalError => ObjectFileStatus::Other, + Err(CacheError::Timeout(_)) => ObjectFileStatus::Timeout, + Err(CacheError::Malformed(_)) => ObjectFileStatus::Malformed, + Err(CacheError::InternalError) => ObjectFileStatus::Other, } } @@ -119,7 +119,7 @@ impl ModuleLookup { .map(|(module_index, object_info)| ModuleEntry { module_index, object_info, - cache: CacheEntry::NotFound, + cache: Err(CacheError::NotFound), source_object: None, }) .collect(); @@ -269,7 +269,7 @@ impl ModuleLookup { entry.object_info.candidates.merge(&candidates); entry.object_info.debug_status = object_file_status_from_cache_entry(&file); - if let CacheEntry::AllGood(CacheFileEntry::SymCache(ref symcache)) = file { + if let Ok(CacheFileEntry::SymCache(ref symcache)) = file { entry.object_info.arch = symcache.get().arch(); } @@ -551,6 +551,6 @@ mod tests { let lookup_result = lookup.lookup_cache(43, AddrMode::Abs).unwrap(); assert_eq!(lookup_result.module_index, 0); assert_eq!(lookup_result.object_info, &info); - assert!(lookup_result.cache.is_not_found()); + assert!(matches!(lookup_result.cache, Err(CacheError::NotFound))); } } diff --git a/crates/symbolicator-service/src/services/symbolication/symbolication.rs b/crates/symbolicator-service/src/services/symbolication/symbolication.rs index 772a8a63f..d83e093b4 100644 --- a/crates/symbolicator-service/src/services/symbolication/symbolication.rs +++ b/crates/symbolicator-service/src/services/symbolication/symbolication.rs @@ -6,7 +6,7 @@ use symbolic::ppdb::PortablePdbCache; use symbolic::symcache::SymCache; use symbolicator_sources::{ObjectType, SourceConfig}; -use crate::cache::CacheEntry; +use crate::cache::CacheError; use crate::types::{ CompleteObjectInfo, CompleteStacktrace, CompletedSymbolicationResponse, FrameStatus, FrameTrust, ObjectFileStatus, RawFrame, RawStacktrace, Registers, Scope, Signal, @@ -136,7 +136,7 @@ fn symbolicate_frame( frame.package = lookup_result.object_info.raw.code_file.clone(); match lookup_result.cache { - CacheEntry::AllGood(CacheFileEntry::SymCache(symcache)) => symbolicate_native_frame( + Ok(CacheFileEntry::SymCache(symcache)) => symbolicate_native_frame( symcache.get(), lookup_result, registers, @@ -144,10 +144,10 @@ fn symbolicate_frame( frame, index, ), - CacheEntry::AllGood(CacheFileEntry::PortablePdbCache(ppdb_cache)) => { + Ok(CacheFileEntry::PortablePdbCache(ppdb_cache)) => { symbolicate_dotnet_frame(ppdb_cache.get(), frame, index) } - CacheEntry::Malformed(_) => Err(FrameStatus::Malformed), + Err(CacheError::Malformed(_)) => Err(FrameStatus::Malformed), _ => Err(FrameStatus::Missing), } } diff --git a/crates/symbolicator-service/src/services/symcaches/mod.rs b/crates/symbolicator-service/src/services/symcaches/mod.rs index e26c325e7..6599054e0 100644 --- a/crates/symbolicator-service/src/services/symcaches/mod.rs +++ b/crates/symbolicator-service/src/services/symcaches/mod.rs @@ -12,7 +12,10 @@ use symbolic::common::{ByteView, SelfCell}; use symbolic::symcache::{self, SymCache, SymCacheConverter}; use symbolicator_sources::{FileType, ObjectId, ObjectType, SourceConfig}; -use crate::cache::{Cache, CacheEntry, CacheStatus, ExpirationTime}; +use crate::cache::{ + cache_entry_as_cache_status, cache_entry_from_cache_status, Cache, CacheEntry, CacheError, + CacheStatus, ExpirationTime, +}; use crate::services::bitcode::BitcodeService; use crate::services::cacher::{CacheItemRequest, CacheKey, CacheVersions, Cacher}; use crate::services::objects::{ @@ -74,8 +77,13 @@ static_assert!(symbolic::symcache::SYMCACHE_VERSION == 8); pub type OwnedSymCache = SelfCell, SymCache<'static>>; -fn parse_symcache_owned(byteview: ByteView<'static>) -> Result { - SelfCell::try_new(byteview, |p| unsafe { SymCache::parse(&*p) }) +fn parse_symcache_owned(byteview: ByteView<'static>) -> Result { + SelfCell::try_new(byteview, |p| unsafe { + SymCache::parse(&*p).map_err(|e| { + tracing::error!(error = %e); + CacheError::InternalError + }) + }) } /// Errors happening while generating a symcache. @@ -109,7 +117,7 @@ pub enum SymCacheError { Timeout, } -impl From<&SymCacheError> for CacheEntry { +impl From<&SymCacheError> for CacheError { fn from(error: &SymCacheError) -> Self { match error { SymCacheError::Io(e) => { @@ -278,8 +286,8 @@ impl CacheItemRequest for FetchSymCacheInternal { data: ByteView<'static>, _expiration: ExpirationTime, ) -> Self::Item { - let cache_entry = CacheEntry::from((status, data)); - cache_entry.try_map(parse_symcache_owned) + let cache_entry = cache_entry_from_cache_status(status, data); + cache_entry.and_then(parse_symcache_owned) } } @@ -302,7 +310,7 @@ pub struct FetchSymCacheResponse { impl Default for FetchSymCacheResponse { fn default() -> Self { Self { - cache: CacheEntry::InternalError, + cache: Err(CacheError::InternalError), candidates: Default::default(), features: Default::default(), } @@ -383,7 +391,7 @@ impl SymCacheActor { handle.source_id(), &handle.uri(), ObjectUseInfo::from_derived_status( - &symcache_file.as_cache_status(), + &cache_entry_as_cache_status(&symcache_file), handle.status(), ), ); @@ -396,14 +404,14 @@ impl SymCacheActor { } } Err(e) => FetchSymCacheResponse { - cache: e.as_ref().into(), + cache: Err(e.as_ref().into()), candidates, features: ObjectFeatures::default(), }, } } None => FetchSymCacheResponse { - cache: CacheEntry::NotFound, + cache: Err(CacheError::NotFound), candidates, features: ObjectFeatures::default(), }, @@ -597,7 +605,7 @@ mod tests { .fetch(fetch_symcache.clone()) .await .cache - .all_good() + .ok() .unwrap(); let symcache = owned_symcache.get(); @@ -628,7 +636,7 @@ mod tests { .fetch(fetch_symcache.clone()) .await .cache - .all_good() + .ok() .unwrap(); let symcache = owned_symcache.get(); @@ -648,7 +656,7 @@ mod tests { .fetch(fetch_symcache.clone()) .await .cache - .all_good() + .ok() .unwrap(); let symcache = owned_symcache.get(); From e1874ecd53f103c0a7d7ec354b9b07a4f25d58a5 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Mon, 5 Dec 2022 15:43:57 +0100 Subject: [PATCH 12/14] Changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7731aace..6f8950608 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Internal + +- Simplified internal error architecture. ([#929](https://github.com/getsentry/symbolicator/pull/929)) + ## 0.6.0 ### Features From 4a4654fc7043fc5f4ef51c924f13a68486b50e8e Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Mon, 5 Dec 2022 15:50:04 +0100 Subject: [PATCH 13/14] Update symbolic dep --- Cargo.lock | 47 +++++++++++++++----------- crates/symbolicator-service/Cargo.toml | 2 +- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8590050aa..38aa77b99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8,7 +8,7 @@ version = "0.17.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b9ecd88a8c8378ca913a680cd98f0f13ac67383d35993f86c90a70e3f137816b" dependencies = [ - "gimli", + "gimli 0.26.2", ] [[package]] @@ -1294,6 +1294,12 @@ name = "gimli" version = "0.26.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "22030e2c5a68ec659fde1e949a745124b48e6fa8b045b7ed5bd1fe4ccc5c4e5d" + +[[package]] +name = "gimli" +version = "0.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dec7af912d60cdbd3677c1af9352ebae6fb8394d165568a2234df0fa00f87793" dependencies = [ "fallible-iterator", "stable_deref_trait", @@ -3436,9 +3442,9 @@ checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" [[package]] name = "symbolic" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ab4dac8fb8ea91e6cd6935ed34220f0c559fe4c8e010a3237415d8ac61746da5" +checksum = "b4e804a19ea4039205b426738671552a7f6bec75015e0437f6318ddb4fb5123c" dependencies = [ "symbolic-cfi", "symbolic-common", @@ -3451,9 +3457,9 @@ dependencies = [ [[package]] name = "symbolic-cfi" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6da324450157034a3de59fe2a892ffa58f6ec6852e1723b9eb91a3ccbb786454" +checksum = "cb7996a16aa672dcc3b129eac5a5ea5f148e80d35a8f904efa3eb8adea5922b3" dependencies = [ "symbolic-common", "symbolic-debuginfo", @@ -3462,9 +3468,9 @@ dependencies = [ [[package]] name = "symbolic-common" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "492875918a6bcbae753a5fc6f39bc87566fc3c25182e76d7f128ef1c8a2375c6" +checksum = "1b55cdc318ede251d0957f07afe5fed912119b8c1bc5a7804151826db999e737" dependencies = [ "debugid", "memmap2", @@ -3475,9 +3481,9 @@ dependencies = [ [[package]] name = "symbolic-debuginfo" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e308a705c38b930758c57ead68d3af6c8ba3f39bd0fdcb69d86a6d11e47f495" +checksum = "10f5d898f6c19efe593df62197f91df6176bdfa664426b16ff9f1a434a83df67" dependencies = [ "bitvec", "dmsort", @@ -3485,7 +3491,7 @@ dependencies = [ "elsa", "fallible-iterator", "flate2", - "gimli", + "gimli 0.27.0", "goblin", "lazy_static", "lazycell", @@ -3507,9 +3513,9 @@ dependencies = [ [[package]] name = "symbolic-demangle" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "214d3a420108471222e55268ef502ba0fc89cfd4de876ff26105c560729127af" +checksum = "79be897be8a483a81fff6a3a4e195b4ac838ef73ca42d348b3f722da9902e489" dependencies = [ "cc", "cpp_demangle", @@ -3520,9 +3526,9 @@ dependencies = [ [[package]] name = "symbolic-il2cpp" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d3e8f6fb713fca935dde72ff1fe7dd45227aa8e939333825206f56a73d0577e4" +checksum = "625c4a9dedade30048ada3162f1c1f63799396698f0146688d77466d93e6f1d4" dependencies = [ "indexmap", "serde_json", @@ -3532,9 +3538,9 @@ dependencies = [ [[package]] name = "symbolic-ppdb" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "df9ed49e7493446437ce0a86f4e3aae69b5d53c7feca960d00c4de3fabfefb2a" +checksum = "125fcd987182e46cd828416a9f2bdb7752c42081b33fa6d80a94afb1fdd4109b" dependencies = [ "indexmap", "symbolic-common", @@ -3545,9 +3551,9 @@ dependencies = [ [[package]] name = "symbolic-symcache" -version = "10.1.2" +version = "10.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3451e79721371b58d2ad76fdaec4700aec21d60c3f0922d60c8f53330d165864" +checksum = "2ffdc2062d9339eabecd0a600cb7d06ddb98b6957734faf10e678c17db838889" dependencies = [ "indexmap", "symbolic-common", @@ -4573,11 +4579,12 @@ dependencies = [ [[package]] name = "wasmparser" -version = "0.94.0" +version = "0.95.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cdac7e1d98d70913ae3b4923dd7419c8ea7bdfd4c44a240a0ba305d929b7f191" +checksum = "f2ea896273ea99b15132414be1da01ab0d8836415083298ecaffbe308eaac87a" dependencies = [ "indexmap", + "url", ] [[package]] diff --git a/crates/symbolicator-service/Cargo.toml b/crates/symbolicator-service/Cargo.toml index 6d29216f3..d04307f68 100644 --- a/crates/symbolicator-service/Cargo.toml +++ b/crates/symbolicator-service/Cargo.toml @@ -38,7 +38,7 @@ sentry = { version = "0.28.0", features = ["tracing"] } serde = { version = "1.0.137", features = ["derive", "rc"] } serde_json = "1.0.81" serde_yaml = "0.9.14" -symbolic = { version = "10.0.0", features = ["cfi", "common-serde", "debuginfo", "demangle", "symcache", "il2cpp", "ppdb"] } +symbolic = { version = "10.2.1", features = ["cfi", "common-serde", "debuginfo", "demangle", "symcache", "il2cpp", "ppdb"] } symbolicator-crash = { path = "../symbolicator-crash", optional = true } symbolicator-sources = { path = "../symbolicator-sources" } tempfile = "3.2.0" From eef2b1eaad6e955c5665e1452cf7e58e4ce40aa8 Mon Sep 17 00:00:00 2001 From: Sebastian Zivota Date: Mon, 5 Dec 2022 16:09:17 +0100 Subject: [PATCH 14/14] Fix doc comment --- crates/symbolicator-service/src/cache.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/symbolicator-service/src/cache.rs b/crates/symbolicator-service/src/cache.rs index e409e06db..2a6181a2e 100644 --- a/crates/symbolicator-service/src/cache.rs +++ b/crates/symbolicator-service/src/cache.rs @@ -207,7 +207,7 @@ impl CacheError { /// Parses a `CacheError` from a byte slice. /// /// * If the slice starts with an error marker, the corresponding error variant will be returned. - /// * If the slice is empty, [`NotFound`](CacheEntry::NotFound) will be returned. + /// * If the slice is empty, [`NotFound`](Self::NotFound) will be returned. /// * Otherwise `None` is returned. fn from_bytes(bytes: &[u8]) -> Option { if let Some(raw_message) = bytes.strip_prefix(CacheError::PERMISSION_DENIED_MARKER) {