From 938a8ebe347639c07042768e97969b75cc600e16 Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Sat, 12 Oct 2024 09:15:10 -0700 Subject: refactor(ext/cache): use concrete error type (#26109) --- ext/cache/Cargo.toml | 1 + ext/cache/lib.rs | 60 +++++++++++++++++++++++++++++++++++++--------------- ext/cache/sqlite.rs | 50 +++++++++++++++++++++++++------------------ 3 files changed, 73 insertions(+), 38 deletions(-) (limited to 'ext/cache') diff --git a/ext/cache/Cargo.toml b/ext/cache/Cargo.toml index 71ca34380..9d876fcb7 100644 --- a/ext/cache/Cargo.toml +++ b/ext/cache/Cargo.toml @@ -19,4 +19,5 @@ deno_core.workspace = true rusqlite.workspace = true serde.workspace = true sha2.workspace = true +thiserror.workspace = true tokio.workspace = true diff --git a/ext/cache/lib.rs b/ext/cache/lib.rs index f6d758b95..08661c349 100644 --- a/ext/cache/lib.rs +++ b/ext/cache/lib.rs @@ -7,7 +7,6 @@ use std::sync::Arc; use async_trait::async_trait; use deno_core::error::type_error; -use deno_core::error::AnyError; use deno_core::op2; use deno_core::serde::Deserialize; use deno_core::serde::Serialize; @@ -19,6 +18,20 @@ use deno_core::ResourceId; mod sqlite; pub use sqlite::SqliteBackedCache; +#[derive(Debug, thiserror::Error)] +pub enum CacheError { + #[error(transparent)] + Sqlite(#[from] rusqlite::Error), + #[error(transparent)] + JoinError(#[from] tokio::task::JoinError), + #[error(transparent)] + Resource(deno_core::error::AnyError), + #[error(transparent)] + Other(deno_core::error::AnyError), + #[error(transparent)] + Io(#[from] std::io::Error), +} + #[derive(Clone)] pub struct CreateCache(pub Arc C>); @@ -92,26 +105,31 @@ pub struct CacheDeleteRequest { pub trait Cache: Clone + 'static { type CacheMatchResourceType: Resource; - async fn storage_open(&self, cache_name: String) -> Result; - async fn storage_has(&self, cache_name: String) -> Result; - async fn storage_delete(&self, cache_name: String) -> Result; + async fn storage_open(&self, cache_name: String) -> Result; + async fn storage_has(&self, cache_name: String) -> Result; + async fn storage_delete( + &self, + cache_name: String, + ) -> Result; /// Put a resource into the cache. async fn put( &self, request_response: CachePutRequest, resource: Option>, - ) -> Result<(), AnyError>; + ) -> Result<(), CacheError>; async fn r#match( &self, request: CacheMatchRequest, ) -> Result< Option<(CacheMatchResponseMeta, Option)>, - AnyError, + CacheError, >; - async fn delete(&self, request: CacheDeleteRequest) - -> Result; + async fn delete( + &self, + request: CacheDeleteRequest, + ) -> Result; } #[op2(async)] @@ -119,7 +137,7 @@ pub trait Cache: Clone + 'static { pub async fn op_cache_storage_open( state: Rc>, #[string] cache_name: String, -) -> Result +) -> Result where CA: Cache, { @@ -131,7 +149,7 @@ where pub async fn op_cache_storage_has( state: Rc>, #[string] cache_name: String, -) -> Result +) -> Result where CA: Cache, { @@ -143,7 +161,7 @@ where pub async fn op_cache_storage_delete( state: Rc>, #[string] cache_name: String, -) -> Result +) -> Result where CA: Cache, { @@ -155,13 +173,19 @@ where pub async fn op_cache_put( state: Rc>, #[serde] request_response: CachePutRequest, -) -> Result<(), AnyError> +) -> Result<(), CacheError> where CA: Cache, { let cache = get_cache::(&state)?; let resource = match request_response.response_rid { - Some(rid) => Some(state.borrow_mut().resource_table.take_any(rid)?), + Some(rid) => Some( + state + .borrow_mut() + .resource_table + .take_any(rid) + .map_err(CacheError::Resource)?, + ), None => None, }; cache.put(request_response, resource).await @@ -172,7 +196,7 @@ where pub async fn op_cache_match( state: Rc>, #[serde] request: CacheMatchRequest, -) -> Result, AnyError> +) -> Result, CacheError> where CA: Cache, { @@ -191,7 +215,7 @@ where pub async fn op_cache_delete( state: Rc>, #[serde] request: CacheDeleteRequest, -) -> Result +) -> Result where CA: Cache, { @@ -199,7 +223,7 @@ where cache.delete(request).await } -pub fn get_cache(state: &Rc>) -> Result +pub fn get_cache(state: &Rc>) -> Result where CA: Cache, { @@ -211,7 +235,9 @@ where state.put(cache); Ok(state.borrow::().clone()) } else { - Err(type_error("CacheStorage is not available in this context")) + Err(CacheError::Other(type_error( + "CacheStorage is not available in this context", + ))) } } diff --git a/ext/cache/sqlite.rs b/ext/cache/sqlite.rs index c3c55dd5e..e4991c32f 100644 --- a/ext/cache/sqlite.rs +++ b/ext/cache/sqlite.rs @@ -30,6 +30,7 @@ use crate::serialize_headers; use crate::vary_header_matches; use crate::Cache; use crate::CacheDeleteRequest; +use crate::CacheError; use crate::CacheMatchRequest; use crate::CacheMatchResponseMeta; use crate::CachePutRequest; @@ -102,7 +103,7 @@ impl Cache for SqliteBackedCache { /// Open a cache storage. Internally, this creates a row in the /// sqlite db if the cache doesn't exist and returns the internal id /// of the cache. - async fn storage_open(&self, cache_name: String) -> Result { + async fn storage_open(&self, cache_name: String) -> Result { let db = self.connection.clone(); let cache_storage_dir = self.cache_storage_dir.clone(); spawn_blocking(move || { @@ -121,14 +122,14 @@ impl Cache for SqliteBackedCache { )?; let responses_dir = get_responses_dir(cache_storage_dir, cache_id); std::fs::create_dir_all(responses_dir)?; - Ok::(cache_id) + Ok::(cache_id) }) .await? } /// Check if a cache with the provided name exists. /// Note: this doesn't check the disk, it only checks the sqlite db. - async fn storage_has(&self, cache_name: String) -> Result { + async fn storage_has(&self, cache_name: String) -> Result { let db = self.connection.clone(); spawn_blocking(move || { let db = db.lock(); @@ -140,13 +141,16 @@ impl Cache for SqliteBackedCache { Ok(count > 0) }, )?; - Ok::(cache_exists) + Ok::(cache_exists) }) .await? } /// Delete a cache storage. Internally, this deletes the row in the sqlite db. - async fn storage_delete(&self, cache_name: String) -> Result { + async fn storage_delete( + &self, + cache_name: String, + ) -> Result { let db = self.connection.clone(); let cache_storage_dir = self.cache_storage_dir.clone(); spawn_blocking(move || { @@ -167,7 +171,7 @@ impl Cache for SqliteBackedCache { std::fs::remove_dir_all(cache_dir)?; } } - Ok::(maybe_cache_id.is_some()) + Ok::(maybe_cache_id.is_some()) }) .await? } @@ -176,10 +180,12 @@ impl Cache for SqliteBackedCache { &self, request_response: CachePutRequest, resource: Option>, - ) -> Result<(), AnyError> { + ) -> Result<(), CacheError> { let db = self.connection.clone(); let cache_storage_dir = self.cache_storage_dir.clone(); - let now = SystemTime::now().duration_since(UNIX_EPOCH)?; + let now = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("SystemTime is before unix epoch"); if let Some(resource) = resource { let body_key = hash(&format!( @@ -193,7 +199,11 @@ impl Cache for SqliteBackedCache { let mut file = tokio::fs::File::create(response_path).await?; let mut buf = BufMutView::new(64 * 1024); loop { - let (size, buf2) = resource.clone().read_byob(buf).await?; + let (size, buf2) = resource + .clone() + .read_byob(buf) + .await + .map_err(CacheError::Other)?; if size == 0 { break; } @@ -224,7 +234,7 @@ impl Cache for SqliteBackedCache { request: CacheMatchRequest, ) -> Result< Option<(CacheMatchResponseMeta, Option)>, - AnyError, + CacheError, > { let db = self.connection.clone(); let cache_storage_dir = self.cache_storage_dir.clone(); @@ -290,19 +300,17 @@ impl Cache for SqliteBackedCache { } Err(err) => return Err(err.into()), }; - return Ok(Some((cache_meta, Some(CacheResponseResource::new(file))))); - } - Some((cache_meta, None)) => { - return Ok(Some((cache_meta, None))); + Ok(Some((cache_meta, Some(CacheResponseResource::new(file))))) } - None => return Ok(None), + Some((cache_meta, None)) => Ok(Some((cache_meta, None))), + None => Ok(None), } } async fn delete( &self, request: CacheDeleteRequest, - ) -> Result { + ) -> Result { let db = self.connection.clone(); spawn_blocking(move || { // TODO(@satyarohith): remove the response body from disk if one exists @@ -311,17 +319,17 @@ impl Cache for SqliteBackedCache { "DELETE FROM request_response_list WHERE cache_id = ?1 AND request_url = ?2", (request.cache_id, &request.request_url), )?; - Ok::(rows_effected > 0) + Ok::(rows_effected > 0) }) .await? } } async fn insert_cache_asset( - db: Arc>, + db: Arc>, put: CachePutRequest, response_body_key: Option, -) -> Result, deno_core::anyhow::Error> { +) -> Result, CacheError> { spawn_blocking(move || { let maybe_response_body = { let db = db.lock(); @@ -339,7 +347,7 @@ async fn insert_cache_asset( response_body_key, put.response_status, put.response_status_text, - SystemTime::now().duration_since(UNIX_EPOCH)?.as_secs(), + SystemTime::now().duration_since(UNIX_EPOCH).expect("SystemTime is before unix epoch").as_secs(), ), |row| { let response_body_key: Option = row.get(0)?; @@ -347,7 +355,7 @@ async fn insert_cache_asset( }, )? }; - Ok::, AnyError>(maybe_response_body) + Ok::, CacheError>(maybe_response_body) }).await? } -- cgit v1.2.3