Repository: ROpdebee/mb-userscripts (ts)

Issue:
`retryTimes` in `@lib/util/async` is broken
Noticed this while working on something else. Because we use an interval, it is possible that for long-running async function calls, it'll start a new try before the old try has completed. We should use timeouts in a loop instead.

Pull request:
fix(lib): wait until function returns before retrying in `retryTimes`
The previous implementation could start to retry a call if the call took a long time to complete.

Closes #223 

Patch:
diff --git a/src/lib/util/async.ts b/src/lib/util/async.ts
index d404be6d..9f6b7adc 100644
--- a/src/lib/util/async.ts
+++ b/src/lib/util/async.ts
@@ -1,35 +1,31 @@
 // Asynchronous utilities
 
-export function asyncTimeout(ms?: number): Promise<void> {
+export function asyncSleep(ms?: number): Promise<void> {
     return new Promise((resolve) => setTimeout(resolve, ms));
 }
 
-export async function retryTimes<T>(fn: () => T, times: number, retryWait: number): Promise<T> {
+export function retryTimes<T>(fn: () => T | Promise<T>, times: number, retryWait: number): Promise<T> {
     if (times <= 0) {
         return Promise.reject(new TypeError('Invalid number of retry times: ' + times));
     }
 
-    // We could've used asyncTimeout to simplify this a lot, but that would
-    // require a lot of mixing of async code and timers, which makes testing
-    // anything which uses this function very difficult.
-    return new Promise((resolve, reject) => {
-        function tryOnce(): void {
-            try {
-                resolve(fn());
-            } catch (err) {
-                if (--times > 0) return;
-                reject(err);
-            }
-            // Stop looping if the function passed, or when it failed but tries
-            // are exhausted. The early return in the catch clause prevents
-            // this statement from executing if the tries aren't exhausted yet.
-            clearInterval(interval);
+    async function createTryPromise(triesLeft: number): Promise<T> {
+        try {
+            // Need to await if the provided function returns a promise, we
+            // want to catch if the promise fails. If it doesn't return a
+            // promise, await does nothing.
+            return await fn();
+        } catch (err) {
+            // If we failed the last attempt, the whole thing fails.
+            if (triesLeft <= 1) throw err;
+
+            // Return a new promise after sleeping. Because of chaining, the
+            // state of our current promise will be the state of this new
+            // promise after it settles.
+            return asyncSleep(retryWait)
+                .then(() => createTryPromise(triesLeft - 1));
         }
+    }
 
-        const interval = setInterval(tryOnce, retryWait);
-        // Manually calling the first try, the one in the interval will be first
-        // called after the first wait period. If the call succeeds immediately,
-        // the interval will be cleared before the first execution happens.
-        tryOnce();
-    });
+    return createTryPromise(times);
 }


Test patch:
diff --git a/tests/lib/util/async.test.ts b/tests/lib/util/async.test.ts
index 5a718219..4967efc5 100644
--- a/tests/lib/util/async.test.ts
+++ b/tests/lib/util/async.test.ts
@@ -1,4 +1,4 @@
-import { asyncTimeout, retryTimes } from '@lib/util/async';
+import { asyncSleep, retryTimes } from '@lib/util/async';
 
 afterEach(() => {
     jest.useRealTimers();
@@ -8,7 +8,7 @@ describe('async timeout', () => {
 
     it('resolves after a timeout', async () => {
         jest.useFakeTimers();
-        const promise = asyncTimeout(500);
+        const promise = asyncSleep(500);
         const spy = jest.fn();
         promise.then(spy);
 
@@ -24,7 +24,7 @@ describe('async timeout', () => {
 
     it('resolves after the correct timeout', async () => {
         jest.useFakeTimers();
-        const promise = asyncTimeout(500);
+        const promise = asyncSleep(500);
         const spy = jest.fn();
         promise.then(spy);
 
@@ -95,6 +95,10 @@ describe('retryTimes', () => {
         // the first failure.
         for (let i = 0; i <= 5; i++) {
             jest.runAllTimers();
+            // Give the async code a chance to run
+            await new Promise<void>((resolve) => {
+                resolve();
+            });
         }
 
         await expect(prom).toReject();