diff --git a/js/__tests__/retryWithBackoff.test.js b/js/__tests__/retryWithBackoff.test.js new file mode 100644 index 0000000000..f687e79e44 --- /dev/null +++ b/js/__tests__/retryWithBackoff.test.js @@ -0,0 +1,351 @@ +/** + * @license + * MusicBlocks v3.4.1 + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +const { retryWithBackoff } = require("../utils/retryWithBackoff"); + +/** + * A no-op delay function that resolves immediately. + * Injected into retryWithBackoff to avoid real timers in tests. + */ +const instantDelay = () => Promise.resolve(); + +describe("retryWithBackoff", () => { + describe("immediate success (no retries needed)", () => { + it("should resolve immediately when check returns truthy on first call", async () => { + const onSuccess = jest.fn(); + const check = jest.fn(() => ({ x: 0, y: 0, width: 100, height: 100 })); + + const result = await retryWithBackoff({ + check, + onSuccess, + delayFn: instantDelay, + maxRetries: 5 + }); + + expect(check).toHaveBeenCalledTimes(1); + expect(onSuccess).toHaveBeenCalledTimes(1); + expect(onSuccess).toHaveBeenCalledWith({ x: 0, y: 0, width: 100, height: 100 }); + expect(result).toEqual({ x: 0, y: 0, width: 100, height: 100 }); + }); + + it("should pass the check result to onSuccess", async () => { + const bounds = { x: 10, y: 20, width: 200, height: 300 }; + const onSuccess = jest.fn(); + + await retryWithBackoff({ + check: () => bounds, + onSuccess, + delayFn: instantDelay + }); + + expect(onSuccess).toHaveBeenCalledWith(bounds); + }); + + it("should not call onRetry when check succeeds immediately", async () => { + const onRetry = jest.fn(); + + await retryWithBackoff({ + check: () => true, + onSuccess: jest.fn(), + onRetry, + delayFn: instantDelay + }); + + expect(onRetry).not.toHaveBeenCalled(); + }); + }); + + describe("retry behavior", () => { + it("should retry when check returns falsy and eventually succeed", async () => { + let callCount = 0; + const check = jest.fn(() => { + callCount++; + // Fail first 3 times, succeed on 4th + return callCount >= 4 ? { ready: true } : null; + }); + const onSuccess = jest.fn(); + + await retryWithBackoff({ + check, + onSuccess, + delayFn: instantDelay, + maxRetries: 10 + }); + + expect(check).toHaveBeenCalledTimes(4); + expect(onSuccess).toHaveBeenCalledWith({ ready: true }); + }); + + it("should call onRetry before each retry attempt with the attempt number", async () => { + let callCount = 0; + const onRetry = jest.fn(); + + await retryWithBackoff({ + check: () => { + callCount++; + return callCount >= 3 ? true : null; + }, + onSuccess: jest.fn(), + onRetry, + delayFn: instantDelay, + maxRetries: 10 + }); + + // 2 retries before success on 3rd check + expect(onRetry).toHaveBeenCalledTimes(2); + expect(onRetry).toHaveBeenNthCalledWith(1, 0); + expect(onRetry).toHaveBeenNthCalledWith(2, 1); + }); + + it("should work without onRetry callback", async () => { + let callCount = 0; + + const result = await retryWithBackoff({ + check: () => { + callCount++; + return callCount >= 2 ? "done" : null; + }, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 5 + }); + + expect(result).toBe("done"); + }); + + it("should call delayFn with exponentially increasing delays", async () => { + let callCount = 0; + const delayFn = jest.fn(() => Promise.resolve()); + + await retryWithBackoff({ + check: () => { + callCount++; + return callCount >= 4 ? true : null; + }, + onSuccess: jest.fn(), + delayFn, + maxRetries: 10, + initialDelay: 50 + }); + + // 3 retries: delays should be 50*2^0=50, 50*2^1=100, 50*2^2=200 + expect(delayFn).toHaveBeenCalledTimes(3); + expect(delayFn).toHaveBeenNthCalledWith(1, 50); + expect(delayFn).toHaveBeenNthCalledWith(2, 100); + expect(delayFn).toHaveBeenNthCalledWith(3, 200); + }); + }); + + describe("max retries exceeded", () => { + it("should reject with an error when max retries are exhausted", async () => { + const check = jest.fn(() => null); + + await expect( + retryWithBackoff({ + check, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 3, + errorMessage: "COULD NOT CREATE CACHE" + }) + ).rejects.toThrow("COULD NOT CREATE CACHE"); + + // check called for attempts 0, 1, 2, 3 then fail on count=4 > maxRetries=3 + expect(check).toHaveBeenCalledTimes(4); + }); + + it("should use default error message when none provided", async () => { + await expect( + retryWithBackoff({ + check: () => null, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 0 + }) + ).rejects.toThrow("Retry limit exceeded"); + }); + + it("should use custom error message", async () => { + await expect( + retryWithBackoff({ + check: () => null, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 0, + errorMessage: "Turtle._createCache: bounds unavailable" + }) + ).rejects.toThrow("Turtle._createCache: bounds unavailable"); + }); + }); + + describe("default parameters", () => { + it("should default maxRetries to 20", async () => { + let callCount = 0; + + // Succeed on attempt 19 (within default 20) + await retryWithBackoff({ + check: () => { + callCount++; + return callCount > 19 ? true : null; + }, + onSuccess: jest.fn(), + delayFn: instantDelay + }); + + expect(callCount).toBe(20); + }); + + it("should default initialDelay to 50ms", async () => { + let callCount = 0; + const delayFn = jest.fn(() => Promise.resolve()); + + await retryWithBackoff({ + check: () => { + callCount++; + return callCount >= 2 ? true : null; + }, + onSuccess: jest.fn(), + delayFn + }); + + // First delay should be 50 * 2^0 = 50 + expect(delayFn).toHaveBeenCalledWith(50); + }); + }); + + describe("error propagation", () => { + it("should reject if check throws an error", async () => { + await expect( + retryWithBackoff({ + check: () => { + throw new Error("check exploded"); + }, + onSuccess: jest.fn(), + delayFn: instantDelay + }) + ).rejects.toThrow("check exploded"); + }); + + it("should reject if onSuccess throws an error", async () => { + await expect( + retryWithBackoff({ + check: () => true, + onSuccess: () => { + throw new Error("onSuccess failed"); + }, + delayFn: instantDelay + }) + ).rejects.toThrow("onSuccess failed"); + }); + + it("should reject if async onSuccess rejects", async () => { + await expect( + retryWithBackoff({ + check: () => true, + onSuccess: async () => { + throw new Error("async onSuccess failed"); + }, + delayFn: instantDelay + }) + ).rejects.toThrow("async onSuccess failed"); + }); + }); + + describe("edge cases", () => { + it("should handle check returning various truthy values", async () => { + const onSuccess = jest.fn(); + + // Number truthy + await retryWithBackoff({ + check: () => 42, + onSuccess, + delayFn: instantDelay + }); + expect(onSuccess).toHaveBeenCalledWith(42); + + // String truthy + onSuccess.mockClear(); + await retryWithBackoff({ + check: () => "ready", + onSuccess, + delayFn: instantDelay + }); + expect(onSuccess).toHaveBeenCalledWith("ready"); + + // Array truthy + onSuccess.mockClear(); + await retryWithBackoff({ + check: () => [1, 2, 3], + onSuccess, + delayFn: instantDelay + }); + expect(onSuccess).toHaveBeenCalledWith([1, 2, 3]); + }); + + it("should treat 0, empty string, and false as falsy (trigger retry)", async () => { + let callCount = 0; + + // Returns 0 first, then true + await retryWithBackoff({ + check: () => { + callCount++; + if (callCount === 1) return 0; + if (callCount === 2) return ""; + if (callCount === 3) return false; + return "success"; + }, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 10 + }); + + expect(callCount).toBe(4); + }); + + it("should handle maxRetries of 0 (fail immediately if check is falsy)", async () => { + const check = jest.fn(() => null); + + await expect( + retryWithBackoff({ + check, + onSuccess: jest.fn(), + delayFn: instantDelay, + maxRetries: 0 + }) + ).rejects.toThrow(); + }); + + it("should handle maxRetries of 1 (one retry allowed)", async () => { + let callCount = 0; + const onSuccess = jest.fn(); + + await retryWithBackoff({ + check: () => { + callCount++; + return callCount >= 2 ? true : null; + }, + onSuccess, + delayFn: instantDelay, + maxRetries: 1 + }); + + expect(callCount).toBe(2); + expect(onSuccess).toHaveBeenCalled(); + }); + }); +}); diff --git a/js/activity.js b/js/activity.js index 67b3c35fa0..46a0a05f78 100644 --- a/js/activity.js +++ b/js/activity.js @@ -71,6 +71,7 @@ let MYDEFINES = [ // 'mespeak', "Chart", "utils/utils", + "utils/retryWithBackoff", "activity/artwork", "widgets/status", "widgets/help", diff --git a/js/block.js b/js/block.js index f00bea3cd1..1ab409ba3a 100644 --- a/js/block.js +++ b/js/block.js @@ -13,8 +13,8 @@ /* global - _, ACCIDENTALLABELS, ACCIDENTALNAMES, addTemperamentToDictionary, - blockBlocks, COLLAPSEBUTTON, COLLAPSETEXTX, COLLAPSETEXTY, + _, addTemperamentToDictionary, + ACCIDENTALLABELS, ACCIDENTALNAMES, blockBlocks, COLLAPSEBUTTON, COLLAPSETEXTX, COLLAPSETEXTY, createjs, DEFAULTACCIDENTAL, DEFAULTDRUM, DEFAULTEFFECT, DEFAULTFILTERTYPE, DEFAULTINTERVAL, DEFAULTINVERT, DEFAULTMODE, DEFAULTNOISE, DEFAULTOSCILLATORTYPE, DEFAULTTEMPERAMENT, @@ -31,7 +31,7 @@ piemenuBoolean, piemenuColor, piemenuCustomNotes, piemenuIntervals, piemenuModes, piemenuNoteValue, piemenuNumber, piemenuPitches, piemenuVoices, piemenuChords, platformColor, ProtoBlock, RSYMBOLS, - safeSVG, SCALENOTES, SHARP, SOLFATTRS, SOLFNOTES, splitScaleDegree, + retryWithBackoff, safeSVG, SCALENOTES, SHARP, SOLFATTRS, SOLFNOTES, splitScaleDegree, splitSolfege, STANDARDBLOCKHEIGHT, TEXTX, TEXTY, topBlock, updateTemperaments, VALUETEXTX, DEFAULTCHORD, VOICENAMES, WESTERN2EISOLFEGENAMES, _THIS_IS_TURTLE_BLOCKS_ @@ -351,42 +351,24 @@ class Block { */ _createCache(callback, args) { const that = this; - return new Promise((resolve, reject) => { - let loopCount = 0; - const MAX_RETRIES = 20; - const INITIAL_DELAY = 50; - - const checkBounds = async counter => { - try { - if (counter !== undefined) { - loopCount = counter; - } - if (loopCount > MAX_RETRIES) { - throw new Error("COULD NOT CREATE CACHE"); - } - - that.bounds = that.container.getBounds(); - - if (that.bounds === null) { - const delayTime = INITIAL_DELAY * Math.pow(2, loopCount); - await delayExecution(delayTime); - that.regenerateArtwork(true, []); - checkBounds(loopCount + 1); - } else { - that.container.cache( - that.bounds.x, - that.bounds.y, - that.bounds.width, - that.bounds.height - ); - callback(that, args); - resolve(); - } - } catch (e) { - reject(e); - } - }; - checkBounds(); + const MAX_RETRIES = 20; + const INITIAL_DELAY = 50; + + return retryWithBackoff({ + check: () => { + that.bounds = that.container.getBounds(); + return that.bounds; + }, + onSuccess: bounds => { + that.container.cache(bounds.x, bounds.y, bounds.width, bounds.height); + callback(that, args); + }, + onRetry: () => { + that.regenerateArtwork(true, []); + }, + maxRetries: MAX_RETRIES, + initialDelay: INITIAL_DELAY, + errorMessage: "COULD NOT CREATE CACHE" }); } @@ -398,35 +380,18 @@ class Block { */ updateCache() { const that = this; - return new Promise((resolve, reject) => { - let loopCount = 0; - const MAX_RETRIES = 15; - const INITIAL_DELAY = 100; - - const updateBounds = async counter => { - try { - if (counter !== undefined) { - loopCount = counter; - } - - if (loopCount > MAX_RETRIES) { - throw new Error("COULD NOT UPDATE CACHE"); - } + const MAX_RETRIES = 15; + const INITIAL_DELAY = 100; - if (that.bounds === null) { - const delayTime = INITIAL_DELAY * Math.pow(2, loopCount); - await that.pause(delayTime); - updateBounds(loopCount + 1); - } else { - that.container.updateCache(); - that.activity.refreshCanvas(); - resolve(); - } - } catch (e) { - reject(e); - } - }; - updateBounds(); + return retryWithBackoff({ + check: () => that.bounds !== null, + onSuccess: () => { + that.container.updateCache(); + that.activity.refreshCanvas(); + }, + maxRetries: MAX_RETRIES, + initialDelay: INITIAL_DELAY, + errorMessage: "COULD NOT UPDATE CACHE" }); } diff --git a/js/loader.js b/js/loader.js index 5ddca264b0..238afecb5a 100644 --- a/js/loader.js +++ b/js/loader.js @@ -53,12 +53,16 @@ requirejs.config({ deps: ["utils/platformstyle"], exports: "_" }, + "utils/retryWithBackoff": { + deps: ["utils/utils"], + exports: "retryWithBackoff" + }, "activity/turtledefs": { deps: ["utils/utils"], exports: "createDefaultStack" }, "activity/block": { - deps: ["activity/turtledefs"], + deps: ["activity/turtledefs", "utils/retryWithBackoff"], exports: "Block" }, "activity/blocks": { @@ -72,7 +76,12 @@ requirejs.config({ exports: "Painter" }, "activity/turtle": { - deps: ["activity/turtledefs", "activity/turtle-singer", "activity/turtle-painter"], + deps: [ + "activity/turtledefs", + "activity/turtle-singer", + "activity/turtle-painter", + "utils/retryWithBackoff" + ], exports: "Turtle" }, "activity/turtles": { diff --git a/js/turtle.js b/js/turtle.js index 32a9854229..43402e17d3 100644 --- a/js/turtle.js +++ b/js/turtle.js @@ -21,7 +21,7 @@ globals createjs, DEFAULTVOLUME, delayExecution, importMembers, Painter, Singer, - DEFAULTVOICE + DEFAULTVOICE, retryWithBackoff */ /* exported Turtle */ /** @@ -87,43 +87,69 @@ class Turtle { /** * Internal function for creating cache. - * Includes workaround for a race condition. + * Uses bounded retry with exponential backoff to handle the race + * condition where container bounds are not yet available. * * @private + * @returns {Promise} Resolves when cache is created, rejects after max retries. */ _createCache() { - this.bounds = this.container.getBounds(); - - if (this.bounds == null) { - setTimeout(() => { - this._createCache(); - }, 200); - } else { - this.container.cache( - this.bounds.x, - this.bounds.y, - this.bounds.width, - this.bounds.height - ); - } + const that = this; + const MAX_RETRIES = 20; + const INITIAL_DELAY = 50; + + return retryWithBackoff({ + check: () => { + that.bounds = that.container.getBounds(); + return that.bounds; + }, + onSuccess: bounds => { + that.container.cache(bounds.x, bounds.y, bounds.width, bounds.height); + }, + maxRetries: MAX_RETRIES, + initialDelay: INITIAL_DELAY, + errorMessage: + "Turtle._createCache: could not get container bounds after " + + MAX_RETRIES + + " attempts" + }); } /** * Internal function for updating cache. - * Includes workaround for a race condition. + * Uses bounded retry with exponential backoff to handle the race + * condition where bounds are not yet available. * - * @async - */ - async updateCache() { - if (this.bounds == null) { - // eslint-disable-next-line no-console - console.debug("Block container for " + this.name + " not yet ready."); - await delayExecution(300); - this.updateCache(); - } else { - this.container.updateCache(); - this.activity.refreshCanvas(); - } + * @returns {Promise} Resolves when cache is updated, rejects after max retries. + */ + updateCache() { + const that = this; + const MAX_RETRIES = 15; + const INITIAL_DELAY = 100; + + return retryWithBackoff({ + check: () => that.bounds !== null, + onSuccess: () => { + that.container.updateCache(); + that.activity.refreshCanvas(); + }, + onRetry: attempt => { + // eslint-disable-next-line no-console + console.debug( + "Turtle container for " + + that.name + + " not yet ready (attempt " + + (attempt + 1) + + "/" + + MAX_RETRIES + + ")" + ); + }, + maxRetries: MAX_RETRIES, + initialDelay: INITIAL_DELAY, + errorMessage: + "Turtle.updateCache: bounds not available after " + MAX_RETRIES + " attempts" + }); } /** @@ -986,3 +1012,7 @@ Turtle.TurtleView = class { if (typeof window !== "undefined") { window.Turtle = Turtle; } + +if (typeof module !== "undefined" && module.exports) { + module.exports = Turtle; +} diff --git a/js/utils/retryWithBackoff.js b/js/utils/retryWithBackoff.js new file mode 100644 index 0000000000..1206323186 --- /dev/null +++ b/js/utils/retryWithBackoff.js @@ -0,0 +1,116 @@ +/** + * @license + * MusicBlocks v3.4.1 + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +/** + * @file Shared retry-with-exponential-backoff utility for handling race conditions + * where resources (e.g., container bounds) may not be immediately available. + * + * This replaces ad-hoc unbounded retry loops that previously existed in + * turtle.js and block.js, providing a consistent, bounded, and testable + * retry mechanism across the codebase. + * + * @author Music Blocks Contributors + */ + +/* exported retryWithBackoff */ + +/** + * Retries an asynchronous check function with exponential backoff until it + * succeeds or the maximum number of retries is exceeded. + * + * The delay between retries grows exponentially: initialDelay * 2^attempt. + * This prevents tight polling loops while still recovering quickly from + * brief race conditions. + * + * @param {Object} options - Configuration for the retry behavior. + * @param {Function} options.check - A function that returns a truthy value when the + * condition is met, or a falsy value to trigger a retry. May be async. + * @param {Function} options.onSuccess - Called with the truthy result of `check` when + * it succeeds. May be async. + * @param {Function} [options.onRetry] - Optional callback invoked before each retry + * attempt, receiving the current attempt number (0-indexed). Useful for + * side effects like regenerating artwork. + * @param {Function} [options.delayFn] - The delay function to use. Defaults to a + * Promise-based setTimeout wrapper. Injected for testability. + * @param {number} [options.maxRetries=20] - Maximum number of retry attempts before + * rejecting with an error. + * @param {number} [options.initialDelay=50] - Initial delay in milliseconds before + * the first retry. Subsequent delays double each attempt. + * @param {string} [options.errorMessage="Retry limit exceeded"] - Error message used + * when max retries are exhausted. + * @returns {Promise} Resolves when `check` returns truthy and `onSuccess` + * completes. Rejects with an Error if retries are exhausted. + * + * @example + * // Basic usage: wait for container bounds to become available + * await retryWithBackoff({ + * check: () => container.getBounds(), + * onSuccess: (bounds) => container.cache(bounds.x, bounds.y, bounds.width, bounds.height), + * maxRetries: 20, + * initialDelay: 50, + * errorMessage: "COULD NOT CREATE CACHE" + * }); + * + * @example + * // With onRetry callback for side effects + * await retryWithBackoff({ + * check: () => container.getBounds(), + * onSuccess: (bounds) => container.cache(bounds.x, bounds.y, bounds.width, bounds.height), + * onRetry: (attempt) => regenerateArtwork(true, []), + * maxRetries: 15, + * initialDelay: 100 + * }); + */ +const retryWithBackoff = async ({ + check, + onSuccess, + onRetry, + delayFn, + maxRetries = 20, + initialDelay = 50, + errorMessage = "Retry limit exceeded" +}) => { + // Default delay function: Promise-based setTimeout + const delay = + delayFn || + (ms => + new Promise(resolve => { + setTimeout(resolve, ms); + })); + + for (let count = 0; count <= maxRetries; count++) { + const result = check(); + + if (result) { + await onSuccess(result); + return result; + } + + if (typeof onRetry === "function") { + onRetry(count); + } + + await delay(initialDelay * Math.pow(2, count)); + } + + throw new Error(errorMessage); +}; + +if (typeof module !== "undefined" && module.exports) { + module.exports = { retryWithBackoff }; +}