refactor(ext/cron): use concrete error type (denoland#26135)

Author: crowlKats

Cargo.lock

@@ -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

ext/cron/Cargo.toml

@@ -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<Self::EH, AnyError>;
+  fn create(&self, spec: CronSpec) -> Result<Self::EH, CronError>;
 }
 
 #[async_trait(?Send)]
 pub trait CronHandle {
-  async fn next(&self, prev_success: bool) -> Result<bool, AnyError>;
+  async fn next(&self, prev_success: bool) -> Result<bool, CronError>;
   fn close(&self);
 }
 

ext/cron/interface.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,14 +46,36 @@ impl<EH: CronHandle + 'static> Resource for CronResource<EH> {
   }
 }
 
+#[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<C>(
   state: Rc<RefCell<OpState>>,
   #[string] name: String,
   #[string] cron_schedule: String,
   #[serde] backoff_schedule: Option<Vec<u32>>,
-) -> Result<ResourceId, AnyError>
+) -> Result<ResourceId, CronError>
 where
   C: CronHandler + 'static,
 {
@@ -90,7 +109,7 @@ async fn op_cron_next<C>(
   state: Rc<RefCell<OpState>>,
   #[smi] rid: ResourceId,
   prev_success: bool,
-) -> Result<bool, AnyError>
+) -> Result<bool, CronError>
 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(())
 }

ext/cron/lib.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<RefCell<RuntimeState>>,
     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<Vec<(String, WeakSender<()>)>, AnyError> {
+  ) -> Result<Vec<(String, WeakSender<()>)>, 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<Self::EH, AnyError> {
+  fn create(&self, spec: CronSpec) -> Result<Self::EH, CronError> {
     // 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::<saffron::Cron>()
-      .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<bool, AnyError> {
+  async fn next(&self, prev_success: bool) -> Result<bool, CronError> {
     self.inner.borrow_mut().permit.take();
 
     if self
@@ -300,7 +299,7 @@ impl CronHandle for CronExecutionHandle {
   }
 }
 
-fn compute_next_deadline(cron_expression: &str) -> Result<u64, AnyError> {
+fn compute_next_deadline(cron_expression: &str) -> Result<u64, CronError> {
   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<u64, AnyError> {
 
   let cron = cron_expression
     .parse::<saffron::Cron>()
-    .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(())
 }

ext/cron/local.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::<CronError>().map(get_cron_error_class))
     .or_else(|| e.downcast_ref::<CanvasError>().map(get_canvas_error))
     .or_else(|| e.downcast_ref::<CacheError>().map(get_cache_error))
     .or_else(|| {