Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(async/unstable): add isRetriable option to retry (#6196) #6197

Merged
merged 3 commits into from
Nov 22, 2024

Conversation

lionel-rowe
Copy link
Contributor

Closes #6196

@lionel-rowe lionel-rowe requested a review from kt3k as a code owner November 20, 2024 06:01
@github-actions github-actions bot added the async label Nov 20, 2024
@lionel-rowe
Copy link
Contributor Author

CI failing on unrelated unstable_spinner test:

Spinner.color can set each color => ./cli/unstable_spinner_test.ts:142:6
error: AssertionError: Expected actual: "
⠋ 
" to contain: "
⠙ ".
  throw new AssertionError(msg);
        ^
    at assertStringIncludes (file:///D:/a/std/std/assert/string_includes.ts:29:9)
    at file:///D:/a/std/std/cli/unstable_spinner_test.ts:146:3

 FAILURES 

Spinner.color can set each color => ./cli/unstable_spinner_test.ts:142:6

@kt3k
Copy link
Member

kt3k commented Nov 20, 2024

Because we'd like to accept every new feature as unstable feature (ref), can you copy the existing retry.ts/retry_test.ts to unstable_retry.ts/unstable_retry_test.ts and move these changes to there?

@lionel-rowe
Copy link
Contributor Author

Because we'd like to accept every new feature as unstable feature (ref), can you copy the existing retry.ts/retry_test.ts to unstable_retry.ts/unstable_retry_test.ts and move these changes to there?

Done. Current output for git diff --no-index async/retry.ts async/unstable_retry.ts:

diff --git a/async/retry.ts b/async/unstable_retry.ts
index d2f0250c..0dc80c86 100644
--- a/async/retry.ts
+++ b/async/unstable_retry.ts
@@ -2,33 +2,13 @@
 // This module is browser compatible.
 
 import { exponentialBackoffWithJitter } from "./_util.ts";
+import { RetryError } from "./retry.ts";
 
 /**
- * Error thrown in {@linkcode retry} once the maximum number of failed attempts
- * has been reached.
+ * Options for {@linkcode retry}.
  *
- * @example Usage
- * ```ts no-assert ignore
- * import { RetryError } from "@std/async/retry";
- *
- * throw new RetryError({ foo: "bar" }, 3);
- * ```
+ * @experimental **UNSTABLE**: New API, yet to be vetted.
  */
-export class RetryError extends Error {
-  /**
-   * Constructs a new {@linkcode RetryError} instance.
-   *
-   * @param cause the cause for this error.
-   * @param attempts the number of retry attempts made.
-   */
-  constructor(cause: unknown, attempts: number) {
-    super(`Retrying exceeded the maxAttempts (${attempts}).`);
-    this.name = "RetryError";
-    this.cause = cause;
-  }
-}
-
-/** Options for {@linkcode retry}. */
 export interface RetryOptions {
   /**
    * How much to backoff after each retry.
@@ -61,6 +41,16 @@ export interface RetryOptions {
    * @default {1}
    */
   jitter?: number;
+
+  /**
+   * Callback to determine if an error or other thrown value is retriable.
+   *
+   * @default {() => true}
+   *
+   * @param err The thrown error or other value.
+   * @returns `true` if the error is retriable, `false` otherwise.
+   */
+  isRetriable?: (err: unknown) => boolean;
 }
 
 /**
@@ -68,6 +58,8 @@ export interface RetryOptions {
  * Retries as long as the given function throws. If the attempts are exhausted,
  * throws a {@linkcode RetryError} with `cause` set to the inner exception.
  *
+ * @experimental **UNSTABLE**: New API, yet to be vetted.
+ *
  * The backoff is calculated by multiplying `minTimeout` with `multiplier` to the power of the current attempt counter (starting at 0 up to `maxAttempts - 1`). It is capped at `maxTimeout` however.
  * How long the actual delay is, depends on `jitter`.
  *
@@ -127,6 +119,7 @@ export async function retry<T>(
     maxAttempts = 5,
     minTimeout = 1000,
     jitter = 1,
+    isRetriable = () => true,
   } = options ?? {};
 
   if (maxTimeout <= 0) {
@@ -150,6 +143,10 @@ export async function retry<T>(
     try {
       return await fn();
     } catch (error) {
+      if (!isRetriable(error)) {
+        throw error;
+      }
+
       if (attempt + 1 >= maxAttempts) {
         throw new RetryError(error, maxAttempts);
       }

@lionel-rowe lionel-rowe changed the title feat(async): add isRetriable option to retry (#6196) feat(async/unstable): add isRetriable option to retry (#6196) Nov 20, 2024
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for updating the PR!

Also the diff above is very helpful!

@kt3k kt3k merged commit a73ff94 into denoland:main Nov 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

async/retry: only retry on certain types of errors
2 participants