cleanup event handlers in looker2d (#4988)

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;

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<State["options"]> = {}
   ) {
+    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<State["options"]>): 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) {

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<SampleId, ModalSampleExtendedWithImage>({
       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();
   }
 }

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;
 };
 

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 <T extends AbstractLooker<BaseState>>(
   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 <T extends AbstractLooker<BaseState>>(
     []
   );
 
+  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 <T extends AbstractLooker<BaseState>>(
           }
         );
 
-        looker.addEventListener("error", (event) => {
-          handleError(event.error);
-        });
+        looker.addEventListener(
+          "error",
+          (event) => {
+            handleError(event.error);
+          },
+          { signal: abortControllerRef.current.signal }
+        );
 
         return looker;
       },