feat(ext/cron) modify Deno.cron API to make handler arg last (#21225)

This PR changes the `Deno.cron` API: * Marks the existing function as deprecated * Introduces 2 new overloads, where the handler arg is always last: ```ts Deno.cron( name: string, schedule: string, handler: () => Promise<void> | void, ) Deno.cron( name: string, schedule: string, options?: { backoffSchedule?: number[]; signal?: AbortSignal }, handler: () => Promise<void> | void, ) ``` This PR also fixes a bug, when other crons continue execution after one of the crons was closed using `signal`.
denoland · Nov 17, 2023 · 66c64ac · 66c64ac
1 parent fdc50d1
commit 66c64ac
Show file tree

Hide file tree

Showing 4 changed files with 143 additions and 32 deletions.
diff --git a/cli/tests/unit/cron_test.ts b/cli/tests/unit/cron_test.ts
@@ -89,17 +89,18 @@ Deno.test(function invalidScheduleTest() {
 Deno.test(function invalidBackoffScheduleTest() {
   assertThrows(
     () =>
-      Deno.cron("abc", "*/1 * * * *", () => {}, {
-        backoffSchedule: [1, 1, 1, 1, 1, 1],
-      }),
+      Deno.cron(
+        "abc",
+        "*/1 * * * *",
+        { backoffSchedule: [1, 1, 1, 1, 1, 1] },
+        () => {},
+      ),
     TypeError,
     "Invalid backoff schedule",
   );
   assertThrows(
     () =>
-      Deno.cron("abc", "*/1 * * * *", () => {}, {
-        backoffSchedule: [3600001],
-      }),
+      Deno.cron("abc", "*/1 * * * *", { backoffSchedule: [3600001] }, () => {}),
     TypeError,
     "Invalid backoff schedule",
   );
@@ -109,16 +110,19 @@ Deno.test(async function tooManyCrons() {
   const crons: Promise<void>[] = [];
   const ac = new AbortController();
   for (let i = 0; i <= 100; i++) {
-    const c = Deno.cron(`abc_${i}`, "*/1 * * * *", () => {}, {
-      signal: ac.signal,
-    });
+    const c = Deno.cron(
+      `abc_${i}`,
+      "*/1 * * * *",
+      { signal: ac.signal },
+      () => {},
+    );
     crons.push(c);
   }
 
   try {
     assertThrows(
       () => {
-        Deno.cron("next-cron", "*/1 * * * *", () => {}, { signal: ac.signal });
+        Deno.cron("next-cron", "*/1 * * * *", { signal: ac.signal }, () => {});
       },
       TypeError,
       "Too many crons",
@@ -133,8 +137,7 @@ Deno.test(async function tooManyCrons() {
 
 Deno.test(async function duplicateCrons() {
   const ac = new AbortController();
-  const c = Deno.cron("abc", "*/20 * * * *", () => {
-  }, { signal: ac.signal });
+  const c = Deno.cron("abc", "*/20 * * * *", { signal: ac.signal }, () => {});
   try {
     assertThrows(
       () => Deno.cron("abc", "*/20 * * * *", () => {}),
@@ -153,12 +156,12 @@ Deno.test(async function basicTest() {
   let count = 0;
   const { promise, resolve } = Promise.withResolvers<void>();
   const ac = new AbortController();
-  const c = Deno.cron("abc", "*/20 * * * *", () => {
+  const c = Deno.cron("abc", "*/20 * * * *", { signal: ac.signal }, () => {
     count++;
     if (count > 5) {
       resolve();
     }
-  }, { signal: ac.signal });
+  });
   try {
     await promise;
   } finally {
@@ -179,18 +182,18 @@ Deno.test(async function multipleCrons() {
     void
   >();
   const ac = new AbortController();
-  const c0 = Deno.cron("abc", "*/20 * * * *", () => {
+  const c0 = Deno.cron("abc", "*/20 * * * *", { signal: ac.signal }, () => {
     count0++;
     if (count0 > 5) {
       resolve0();
     }
-  }, { signal: ac.signal });
-  const c1 = Deno.cron("xyz", "*/20 * * * *", () => {
+  });
+  const c1 = Deno.cron("xyz", "*/20 * * * *", { signal: ac.signal }, () => {
     count1++;
     if (count1 > 5) {
       resolve1();
     }
-  }, { signal: ac.signal });
+  });
   try {
     await promise0;
     await promise1;
@@ -212,11 +215,16 @@ Deno.test(async function overlappingExecutions() {
     void
   >();
   const ac = new AbortController();
-  const c = Deno.cron("abc", "*/20 * * * *", async () => {
-    resolve0();
-    count++;
-    await promise1;
-  }, { signal: ac.signal });
+  const c = Deno.cron(
+    "abc",
+    "*/20 * * * *",
+    { signal: ac.signal },
+    async () => {
+      resolve0();
+      count++;
+      await promise1;
+    },
+  );
   try {
     await promise0;
   } finally {
@@ -228,16 +236,44 @@ Deno.test(async function overlappingExecutions() {
   assertEquals(count, 1);
 });
 
-Deno.test(async function retriesWithBackkoffSchedule() {
+Deno.test(async function retriesWithBackoffSchedule() {
+  Deno.env.set("DENO_CRON_TEST_SCHEDULE_OFFSET", "5000");
+
+  let count = 0;
+  const ac = new AbortController();
+  const c = Deno.cron("abc", "*/20 * * * *", {
+    signal: ac.signal,
+    backoffSchedule: [10, 20],
+  }, async () => {
+    count += 1;
+    await sleep(10);
+    throw new TypeError("cron error");
+  });
+  try {
+    await sleep(6000);
+  } finally {
+    ac.abort();
+    await c;
+  }
+
+  // The cron should have executed 3 times (1st attempt and 2 retries).
+  assertEquals(count, 3);
+});
+
+Deno.test(async function retriesWithBackoffScheduleOldApi() {
   Deno.env.set("DENO_CRON_TEST_SCHEDULE_OFFSET", "5000");
 
   let count = 0;
   const ac = new AbortController();
-  const c = Deno.cron("abc", "*/20 * * * *", async () => {
+  const c = Deno.cron("abc2", "*/20 * * * *", async () => {
     count += 1;
     await sleep(10);
     throw new TypeError("cron error");
-  }, { signal: ac.signal, backoffSchedule: [10, 20] });
+  }, {
+    signal: ac.signal,
+    backoffSchedule: [10, 20],
+  });
+
   try {
     await sleep(6000);
   } finally {

diff --git a/cli/tsc/dts/lib.deno.unstable.d.ts b/cli/tsc/dts/lib.deno.unstable.d.ts
@@ -1334,12 +1334,67 @@ declare namespace Deno {
    * second, 5 seconds, and 10 seconds delay between each retry.
    *
    * @category Cron
+   * @deprecated Use other {@linkcode cron} overloads instead. This overload
+   * will be removed in the future.
    */
   export function cron(
     name: string,
     schedule: string,
     handler: () => Promise<void> | void,
-    options?: { backoffSchedule?: number[]; signal?: AbortSignal },
+    options: { backoffSchedule?: number[]; signal?: AbortSignal },
+  ): Promise<void>;
+
+  /** **UNSTABLE**: New API, yet to be vetted.
+   *
+   * Create a cron job that will periodically execute the provided handler
+   * callback based on the specified schedule.
+   *
+   * ```ts
+   * Deno.cron("sample cron", "20 * * * *", () => {
+   *   console.log("cron job executed");
+   * });
+   * ```
+   *
+   * `schedule` is a Unix cron format expression, where time is specified
+   * using UTC time zone.
+   *
+   * @category Cron
+   */
+  export function cron(
+    name: string,
+    schedule: string,
+    handler: () => Promise<void> | void,
+  ): Promise<void>;
+
+  /** **UNSTABLE**: New API, yet to be vetted.
+   *
+   * Create a cron job that will periodically execute the provided handler
+   * callback based on the specified schedule.
+   *
+   * ```ts
+   * Deno.cron("sample cron", "20 * * * *", {
+   *   backoffSchedule: [10, 20]
+   * }, () => {
+   *   console.log("cron job executed");
+   * });
+   * ```
+   *
+   * `schedule` is a Unix cron format expression, where time is specified
+   * using UTC time zone.
+   *
+   * `backoffSchedule` option can be used to specify the retry policy for failed
+   * executions. Each element in the array represents the number of milliseconds
+   * to wait before retrying the execution. For example, `[1000, 5000, 10000]`
+   * means that a failed execution will be retried at most 3 times, with 1
+   * second, 5 seconds, and 10 seconds delay between each retry.
+   *
+   * @category Cron
+   */
+  export function cron(
+    name: string,
+    schedule: string,
+    options: { backoffSchedule?: number[]; signal?: AbortSignal },
+    handler: () => Promise<void> | void,
   ): Promise<void>;
 
   /** **UNSTABLE**: New API, yet to be vetted.

diff --git a/ext/cron/01_cron.ts b/ext/cron/01_cron.ts
@@ -6,16 +6,33 @@ const core = Deno.core;
 function cron(
   name: string,
   schedule: string,
-  handler: () => Promise<void> | void,
-  options?: { backoffSchedule?: number[]; signal?: AbortSignal },
+  handlerOrOptions1:
+    | (() => Promise<void> | void)
+    | ({ backoffSchedule?: number[]; signal?: AbortSignal }),
+  handlerOrOptions2?:
+    | (() => Promise<void> | void)
+    | ({ backoffSchedule?: number[]; signal?: AbortSignal }),
 ) {
   if (name === undefined) {
     throw new TypeError("Deno.cron requires a unique name");
   }
   if (schedule === undefined) {
     throw new TypeError("Deno.cron requires a valid schedule");
   }
-  if (handler === undefined) {
+
+  let handler: () => Promise<void> | void;
+  let options: { backoffSchedule?: number[]; signal?: AbortSignal } | undefined;
+
+  if (typeof handlerOrOptions1 === "function") {
+    handler = handlerOrOptions1;
+    if (typeof handlerOrOptions2 === "function") {
+      throw new TypeError("options must be an object");
+    }
+    options = handlerOrOptions2;
+  } else if (typeof handlerOrOptions2 === "function") {
+    handler = handlerOrOptions2;
+    options = handlerOrOptions1;
+  } else {
     throw new TypeError("Deno.cron requires a handler");
   }
 

diff --git a/ext/cron/local.rs b/ext/cron/local.rs
@@ -174,8 +174,11 @@ impl RuntimeState {
             .map(move |name| (*ts, name.clone()))
             .collect::<Vec<_>>()
         })
-        .map(|(_, name)| {
-          (name.clone(), self.crons.get(&name).unwrap().next_tx.clone())
+        .filter_map(|(_, name)| {
+          self
+            .crons
+            .get(&name)
+            .map(|c| (name.clone(), c.next_tx.clone()))
         })
         .collect::<Vec<_>>()
     };