diff --git a/app/packages/core/src/components/Modal/Modal.tsx b/app/packages/core/src/components/Modal/Modal.tsx index c7c2b196cf..5b4b0d9ee0 100644 --- a/app/packages/core/src/components/Modal/Modal.tsx +++ b/app/packages/core/src/components/Modal/Modal.tsx @@ -104,6 +104,10 @@ const Modal = () => { } clearModal(); + activeLookerRef.current?.removeEventListener( + "close", + modalCloseHandler + ); }, [clearModal, jsonPanel, helpPanel] ); @@ -183,13 +187,6 @@ const Modal = () => { looker.addEventListener("close", modalCloseHandler); }, []); - // cleanup effect - useEffect(() => { - return () => { - activeLookerRef.current?.removeEventListener("close", modalCloseHandler); - }; - }, []); - const setActiveLookerRef = useCallback( (looker: fos.Lookers) => { activeLookerRef.current = looker; diff --git a/app/packages/looker/src/lookers/abstract.ts b/app/packages/looker/src/lookers/abstract.ts index ad8c70c528..8c253e07bc 100644 --- a/app/packages/looker/src/lookers/abstract.ts +++ b/app/packages/looker/src/lookers/abstract.ts @@ -60,11 +60,17 @@ const getLabelsWorker = (() => { let workers: Worker[]; let next = -1; - return (dispatchEvent) => { + return (dispatchEvent, abortController) => { if (!workers) { workers = []; for (let i = 0; i < numWorkers; i++) { - workers.push(createWorker(LookerUtils.workerCallbacks, dispatchEvent)); + workers.push( + createWorker( + LookerUtils.workerCallbacks, + dispatchEvent, + abortController + ) + ); } } @@ -102,11 +108,14 @@ export abstract class AbstractLooker< private isBatching = false; private isCommittingBatchUpdates = false; + private readonly abortController: AbortController; + constructor( sample: S, config: State["config"], options: Partial = {} ) { + this.abortController = new AbortController(); this.eventTarget = new EventTarget(); this.subscriptions = {}; this.updater = this.makeUpdate(); @@ -383,9 +392,19 @@ export abstract class AbstractLooker< addEventListener( eventType: string, handler: EventListenerOrEventListenerObject | null, - ...args: any[] + optionsOrUseCapture?: boolean | AddEventListenerOptions ) { - this.eventTarget.addEventListener(eventType, handler, ...args); + const argsWithSignal: AddEventListenerOptions = + typeof optionsOrUseCapture === "boolean" + ? { + signal: this.abortController.signal, + capture: optionsOrUseCapture, + } + : { + ...(optionsOrUseCapture ?? {}), + signal: this.abortController.signal, + }; + this.eventTarget.addEventListener(eventType, handler, argsWithSignal); } removeEventListener( @@ -489,6 +508,7 @@ export abstract class AbstractLooker< this.lookerElement.element.parentNode.removeChild( this.lookerElement.element ); + this.abortController.abort(); } abstract updateOptions(options: Partial): void; @@ -674,8 +694,9 @@ export abstract class AbstractLooker< private loadSample(sample: Sample) { const messageUUID = uuid(); - const labelsWorker = getLabelsWorker((event, detail) => - this.dispatchEvent(event, detail) + const labelsWorker = getLabelsWorker( + (event, detail) => this.dispatchEvent(event, detail), + this.abortController ); const listener = ({ data: { sample, coloring, uuid } }) => { if (uuid === messageUUID) { diff --git a/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts b/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts index 641d7f9582..df3fc41e8d 100644 --- a/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts +++ b/app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts @@ -22,8 +22,11 @@ export class ImaVidFrameSamples { private readonly storeBufferManager: BufferManager; + private readonly abortController: AbortController; + constructor(storeBufferManager: BufferManager) { this.storeBufferManager = storeBufferManager; + this.abortController = new AbortController(); this.samples = new LRUCache({ max: MAX_FRAME_SAMPLES_CACHE_SIZE, @@ -65,37 +68,45 @@ export class ImaVidFrameSamples { const source = getSampleSrc(standardizedUrls[mediaField]); return new Promise((resolve) => { - image.addEventListener("load", () => { - const sample = this.samples.get(sampleId); - - if (!sample) { - // sample was removed from the cache, this shouldn't happen... - // but if it does, it might be because the cache was cleared - // todo: handle this case better + image.addEventListener( + "load", + () => { + const sample = this.samples.get(sampleId); + + if (!sample) { + // sample was removed from the cache, this shouldn't happen... + // but if it does, it might be because the cache was cleared + // todo: handle this case better + console.error( + "Sample was removed from cache before image loaded", + sampleId + ); + image.src = BASE64_BLACK_IMAGE; + return; + } + + sample.image = image; + resolve(sampleId); + }, + { signal: this.abortController.signal } + ); + + image.addEventListener( + "error", + () => { console.error( - "Sample was removed from cache before image loaded", - sampleId + "Failed to load image for sample with id", + sampleId, + "at url", + source ); - image.src = BASE64_BLACK_IMAGE; - return; - } - sample.image = image; - resolve(sampleId); - }); - - image.addEventListener("error", () => { - console.error( - "Failed to load image for sample with id", - sampleId, - "at url", - source - ); - - // use a placeholder blank black image to not block animation - // setting src should trigger the load event - image.src = BASE64_BLACK_IMAGE; - }); + // use a placeholder blank black image to not block animation + // setting src should trigger the load event + image.src = BASE64_BLACK_IMAGE; + }, + { signal: this.abortController.signal } + ); image.src = source; }); @@ -124,5 +135,6 @@ export class ImaVidFrameSamples { this.reverseFrameIndex.clear(); this.samples.clear(); this.storeBufferManager.reset(); + this.abortController.abort(); } } diff --git a/app/packages/looker/src/util.ts b/app/packages/looker/src/util.ts index fb8cbd9a00..0209ec3a13 100644 --- a/app/packages/looker/src/util.ts +++ b/app/packages/looker/src/util.ts @@ -446,7 +446,8 @@ export const createWorker = ( listeners?: { [key: string]: ((worker: Worker, args: any) => void)[]; }, - dispatchEvent?: DispatchEvent + dispatchEvent?: DispatchEvent, + abortController?: AbortController ): Worker => { let worker: Worker = null; @@ -456,17 +457,26 @@ export const createWorker = ( worker = new Worker(new URL("./worker/index.ts", import.meta.url)); } - worker.onerror = (error) => { - dispatchEvent("error", error); - }; - worker.addEventListener("message", ({ data }) => { - if (data.error) { - const error = !ERRORS[data.error.cls] - ? new Error(data.error.message) - : new ERRORS[data.error.cls](data.error.data, data.error.message); - dispatchEvent("error", new ErrorEvent("error", { error })); - } - }); + worker.addEventListener( + "error", + (error) => { + dispatchEvent("error", error); + }, + { signal: abortController.signal } + ); + + worker.addEventListener( + "message", + ({ data }) => { + if (data.error) { + const error = !ERRORS[data.error.cls] + ? new Error(data.error.message) + : new ERRORS[data.error.cls](data.error.data, data.error.message); + dispatchEvent("error", new ErrorEvent("error", { error })); + } + }, + { signal: abortController.signal } + ); worker.postMessage({ method: "init", @@ -477,13 +487,17 @@ export const createWorker = ( return worker; } - worker.addEventListener("message", ({ data: { method, ...args } }) => { - if (!(method in listeners)) { - return; - } + worker.addEventListener( + "message", + ({ data: { method, ...args } }) => { + if (!(method in listeners)) { + return; + } - listeners[method].forEach((callback) => callback(worker, args)); - }); + listeners[method].forEach((callback) => callback(worker, args)); + }, + { signal: abortController.signal } + ); return worker; }; diff --git a/app/packages/state/src/hooks/useCreateLooker.ts b/app/packages/state/src/hooks/useCreateLooker.ts index 1fc1b748e3..924348c54c 100644 --- a/app/packages/state/src/hooks/useCreateLooker.ts +++ b/app/packages/state/src/hooks/useCreateLooker.ts @@ -18,7 +18,7 @@ import { isNullish, } from "@fiftyone/utilities"; import { get } from "lodash"; -import { useRef } from "react"; +import { useEffect, useRef } from "react"; import { useErrorHandler } from "react-error-boundary"; import { useRelayEnvironment } from "react-relay"; import { useRecoilCallback, useRecoilValue } from "recoil"; @@ -39,6 +39,7 @@ export default >( highlight?: (sample: Sample) => boolean, enableTimeline?: boolean ) => { + const abortControllerRef = useRef(new AbortController()); const environment = useRelayEnvironment(); const selected = useRecoilValue(selectedSamples); const isClip = useRecoilValue(viewAtoms.isClipsView); @@ -68,6 +69,13 @@ export default >( [] ); + useEffect(() => { + return () => { + // sending abort signal to clean up all event handlers + return abortControllerRef.current.abort(); + }; + }, []); + const create = useRecoilCallback( ({ snapshot }) => ( @@ -248,9 +256,13 @@ export default >( } ); - looker.addEventListener("error", (event) => { - handleError(event.error); - }); + looker.addEventListener( + "error", + (event) => { + handleError(event.error); + }, + { signal: abortControllerRef.current.signal } + ); return looker; },