From 2ac699fe6e5b69f656046c5a9657542dc1cf2e9d Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Sat, 12 Oct 2024 14:23:49 -0700 Subject: [PATCH] refactor(ext/cron): use concrete error type (#26135) --- Cargo.lock | 1 + ext/cron/Cargo.toml | 1 + ext/cron/interface.rs | 6 +++--- ext/cron/lib.rs | 42 +++++++++++++++++++++++++++++------------- ext/cron/local.rs | 31 ++++++++++++++++--------------- runtime/errors.rs | 19 ++++++++++++++++++- 6 files changed, 68 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2500b5a2cd..d1341189f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1467,6 +1467,7 @@ dependencies = [ "chrono", "deno_core", "saffron", + "thiserror", "tokio", ] diff --git a/ext/cron/Cargo.toml b/ext/cron/Cargo.toml index a773521f5b..10f09b57cf 100644 --- a/ext/cron/Cargo.toml +++ b/ext/cron/Cargo.toml @@ -19,4 +19,5 @@ async-trait.workspace = true chrono = { workspace = true, features = ["now"] } deno_core.workspace = true saffron.workspace = true +thiserror.workspace = true tokio.workspace = true diff --git a/ext/cron/interface.rs b/ext/cron/interface.rs index 01b1d17895..a19525cc4e 100644 --- a/ext/cron/interface.rs +++ b/ext/cron/interface.rs @@ -1,17 +1,17 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use crate::CronError; use async_trait::async_trait; -use deno_core::error::AnyError; pub trait CronHandler { type EH: CronHandle + 'static; - fn create(&self, spec: CronSpec) -> Result; + fn create(&self, spec: CronSpec) -> Result; } #[async_trait(?Send)] pub trait CronHandle { - async fn next(&self, prev_success: bool) -> Result; + async fn next(&self, prev_success: bool) -> Result; fn close(&self); } diff --git a/ext/cron/lib.rs b/ext/cron/lib.rs index e350e4d698..feffb5e511 100644 --- a/ext/cron/lib.rs +++ b/ext/cron/lib.rs @@ -7,16 +7,13 @@ use std::borrow::Cow; use std::cell::RefCell; use std::rc::Rc; +pub use crate::interface::*; use deno_core::error::get_custom_error_class; -use deno_core::error::type_error; -use deno_core::error::AnyError; use deno_core::op2; use deno_core::OpState; use deno_core::Resource; use deno_core::ResourceId; -pub use crate::interface::*; - pub const UNSTABLE_FEATURE_NAME: &str = "cron"; deno_core::extension!(deno_cron, @@ -49,6 +46,28 @@ impl Resource for CronResource { } } +#[derive(Debug, thiserror::Error)] +pub enum CronError { + #[error(transparent)] + Resource(deno_core::error::AnyError), + #[error("Cron name cannot exceed 64 characters: current length {0}")] + NameExceeded(usize), + #[error("Invalid cron name: only alphanumeric characters, whitespace, hyphens, and underscores are allowed")] + NameInvalid, + #[error("Cron with this name already exists")] + AlreadyExists, + #[error("Too many crons")] + TooManyCrons, + #[error("Invalid cron schedule")] + InvalidCron, + #[error("Invalid backoff schedule")] + InvalidBackoff, + #[error(transparent)] + AcquireError(#[from] tokio::sync::AcquireError), + #[error(transparent)] + Other(deno_core::error::AnyError), +} + #[op2] #[smi] fn op_cron_create( @@ -56,7 +75,7 @@ fn op_cron_create( #[string] name: String, #[string] cron_schedule: String, #[serde] backoff_schedule: Option>, -) -> Result +) -> Result where C: CronHandler + 'static, { @@ -90,7 +109,7 @@ async fn op_cron_next( state: Rc>, #[smi] rid: ResourceId, prev_success: bool, -) -> Result +) -> Result where C: CronHandler + 'static, { @@ -102,7 +121,7 @@ where if get_custom_error_class(&err) == Some("BadResource") { return Ok(false); } else { - return Err(err); + return Err(CronError::Resource(err)); } } }; @@ -112,17 +131,14 @@ where cron_handler.next(prev_success).await } -fn validate_cron_name(name: &str) -> Result<(), AnyError> { +fn validate_cron_name(name: &str) -> Result<(), CronError> { if name.len() > 64 { - return Err(type_error(format!( - "Cron name cannot exceed 64 characters: current length {}", - name.len() - ))); + return Err(CronError::NameExceeded(name.len())); } if !name.chars().all(|c| { c.is_ascii_whitespace() || c.is_ascii_alphanumeric() || c == '_' || c == '-' }) { - return Err(type_error("Invalid cron name: only alphanumeric characters, whitespace, hyphens, and underscores are allowed")); + return Err(CronError::NameInvalid); } Ok(()) } diff --git a/ext/cron/local.rs b/ext/cron/local.rs index dd60e750a0..1110baadb8 100644 --- a/ext/cron/local.rs +++ b/ext/cron/local.rs @@ -10,8 +10,6 @@ use std::rc::Weak; use std::sync::Arc; use async_trait::async_trait; -use deno_core::error::type_error; -use deno_core::error::AnyError; use deno_core::futures; use deno_core::futures::FutureExt; use deno_core::unsync::spawn; @@ -21,6 +19,7 @@ use tokio::sync::mpsc::WeakSender; use tokio::sync::OwnedSemaphorePermit; use tokio::sync::Semaphore; +use crate::CronError; use crate::CronHandle; use crate::CronHandler; use crate::CronSpec; @@ -81,7 +80,7 @@ impl LocalCronHandler { async fn cron_loop( runtime_state: Rc>, mut cron_schedule_rx: mpsc::Receiver<(String, bool)>, - ) -> Result<(), AnyError> { + ) -> Result<(), CronError> { loop { let earliest_deadline = runtime_state .borrow() @@ -154,7 +153,7 @@ impl LocalCronHandler { impl RuntimeState { fn get_ready_crons( &mut self, - ) -> Result)>, AnyError> { + ) -> Result)>, CronError> { let now = chrono::Utc::now().timestamp_millis() as u64; let ready = { @@ -191,7 +190,7 @@ impl RuntimeState { impl CronHandler for LocalCronHandler { type EH = CronExecutionHandle; - fn create(&self, spec: CronSpec) -> Result { + fn create(&self, spec: CronSpec) -> Result { // Ensure that the cron loop is started. self.cron_loop_join_handle.get_or_init(|| { let (cron_schedule_tx, cron_schedule_rx) = @@ -208,17 +207,17 @@ impl CronHandler for LocalCronHandler { let mut runtime_state = self.runtime_state.borrow_mut(); if runtime_state.crons.len() > MAX_CRONS { - return Err(type_error("Too many crons")); + return Err(CronError::TooManyCrons); } if runtime_state.crons.contains_key(&spec.name) { - return Err(type_error("Cron with this name already exists")); + return Err(CronError::AlreadyExists); } // Validate schedule expression. spec .cron_schedule .parse::() - .map_err(|_| type_error("Invalid cron schedule"))?; + .map_err(|_| CronError::InvalidCron)?; // Validate backoff_schedule. if let Some(backoff_schedule) = &spec.backoff_schedule { @@ -263,7 +262,7 @@ struct Inner { #[async_trait(?Send)] impl CronHandle for CronExecutionHandle { - async fn next(&self, prev_success: bool) -> Result { + async fn next(&self, prev_success: bool) -> Result { self.inner.borrow_mut().permit.take(); if self @@ -300,7 +299,7 @@ impl CronHandle for CronExecutionHandle { } } -fn compute_next_deadline(cron_expression: &str) -> Result { +fn compute_next_deadline(cron_expression: &str) -> Result { let now = chrono::Utc::now(); if let Ok(test_schedule) = env::var("DENO_CRON_TEST_SCHEDULE_OFFSET") { @@ -311,19 +310,21 @@ fn compute_next_deadline(cron_expression: &str) -> Result { let cron = cron_expression .parse::() - .map_err(|_| anyhow::anyhow!("invalid cron expression"))?; + .map_err(|_| CronError::InvalidCron)?; let Some(next_deadline) = cron.next_after(now) else { - return Err(anyhow::anyhow!("invalid cron expression")); + return Err(CronError::InvalidCron); }; Ok(next_deadline.timestamp_millis() as u64) } -fn validate_backoff_schedule(backoff_schedule: &[u32]) -> Result<(), AnyError> { +fn validate_backoff_schedule( + backoff_schedule: &[u32], +) -> Result<(), CronError> { if backoff_schedule.len() > MAX_BACKOFF_COUNT { - return Err(type_error("Invalid backoff schedule")); + return Err(CronError::InvalidBackoff); } if backoff_schedule.iter().any(|s| *s > MAX_BACKOFF_MS) { - return Err(type_error("Invalid backoff schedule")); + return Err(CronError::InvalidBackoff); } Ok(()) } diff --git a/runtime/errors.rs b/runtime/errors.rs index daec653d1e..51d6d96fa3 100644 --- a/runtime/errors.rs +++ b/runtime/errors.rs @@ -16,6 +16,7 @@ use deno_core::error::AnyError; use deno_core::serde_json; use deno_core::url; use deno_core::ModuleResolutionError; +use deno_cron::CronError; use std::env; use std::error::Error; use std::io; @@ -156,6 +157,22 @@ pub fn get_nix_error_class(error: &nix::Error) -> &'static str { } } +pub fn get_cron_error_class(e: &CronError) -> &'static str { + match e { + CronError::Resource(e) => { + deno_core::error::get_custom_error_class(e).unwrap_or("Error") + } + CronError::NameExceeded(_) => "TypeError", + CronError::NameInvalid => "TypeError", + CronError::AlreadyExists => "TypeError", + CronError::TooManyCrons => "TypeError", + CronError::InvalidCron => "TypeError", + CronError::InvalidBackoff => "TypeError", + CronError::AcquireError(_) => "Error", + CronError::Other(e) => get_error_class_name(e).unwrap_or("Error"), + } +} + fn get_canvas_error(e: &CanvasError) -> &'static str { match e { CanvasError::UnsupportedColorType(_) => "TypeError", @@ -194,7 +211,7 @@ pub fn get_error_class_name(e: &AnyError) -> Option<&'static str> { .or_else(|| deno_web::get_error_class_name(e)) .or_else(|| deno_webstorage::get_not_supported_error_class_name(e)) .or_else(|| deno_websocket::get_network_error_class_name(e)) - .or_else(|| deno_websocket::get_network_error_class_name(e)) + .or_else(|| e.downcast_ref::().map(get_cron_error_class)) .or_else(|| e.downcast_ref::().map(get_canvas_error)) .or_else(|| e.downcast_ref::().map(get_cache_error)) .or_else(|| {