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.

Sign up for GitHub

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

Jump to bottom

cleanup event handlers in looker2d #4988

Merged
merged 1 commit into from
Oct 30, 2024
+119 −63
Merged

cleanup event handlers in looker2d #4988

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions app/packages/core/src/components/Modal/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ const Modal = () => {
}

clearModal();
activeLookerRef.current?.removeEventListener(
"close",
modalCloseHandler
);
Comment on lines +107 to +110
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check before removing event listener.

The event listener removal should include a null check to prevent potential runtime errors.

Apply this diff:

-        activeLookerRef.current?.removeEventListener(
-          "close",
-          modalCloseHandler
-        );
+        if (activeLookerRef.current) {
+          activeLookerRef.current.removeEventListener(
+            "close",
+            modalCloseHandler
+          );
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
activeLookerRef.current?.removeEventListener(
"close",
modalCloseHandler
);
if (activeLookerRef.current) {
activeLookerRef.current.removeEventListener(
"close",
modalCloseHandler
);
}

⚠️ Potential issue

Potential memory leak: Add cleanup effect.

The event listener cleanup only occurs when the modal is manually closed. If the component is unmounted without closing (e.g., route change), the event listener will remain attached.

Add a cleanup effect to ensure the event listener is always removed:

+ useEffect(() => {
+   return () => {
+     if (activeLookerRef.current) {
+       activeLookerRef.current.removeEventListener("close", modalCloseHandler);
+     }
+   };
+ }, [modalCloseHandler]);

Committable suggestion was skipped due to low confidence.

},
[clearModal, jsonPanel, helpPanel]
);
Expand Down Expand Up @@ -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;
Expand Down
33 changes: 27 additions & 6 deletions app/packages/looker/src/lookers/abstract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
);
Comment on lines +63 to +73
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Potential issue with shared AbortControllers in worker pool

The getLabelsWorker function uses a shared worker pool across all instances of AbstractLooker. Passing abortController to createWorker during worker initialization might lead to unintended behavior, as the abortController from one instance could affect workers used by other instances. This could result in aborting workers that are still in use elsewhere.

Consider modifying the worker management to ensure that each AbstractLooker instance does not interfere with others. One approach could be to instantiate separate workers for each instance or adjust the design so that the abortController is appropriately scoped and does not conflict across instances.

}
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Comment on lines +697 to +699
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Unintended side effects when aborting shared workers

When calling getLabelsWorker with this.abortController, the retrieved worker from the shared pool might have been initialized with a different AbortController. Calling this.abortController.abort() in one instance of AbstractLooker could inadvertently abort workers that are in use by other instances.

To prevent cross-instance interference, consider revising the worker pool implementation. Options include:

  • Assign unique workers to each AbstractLooker instance.
  • Implement a messaging protocol within workers to handle abort signals specific to each instance.
  • Avoid passing the instance-specific abortController when initializing shared workers.

Would you like assistance in refactoring the worker management to address this issue?

);
const listener = ({ data: { sample, coloring, uuid } }) => {
if (uuid === messageUUID) {
Expand Down
68 changes: 40 additions & 28 deletions app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 }
);
Comment on lines +94 to +109
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing image loading error recovery

The current implementation could be improved by:

  1. Adding retry logic for transient failures
  2. Using structured logging with error categorization
  3. Adding telemetry for failed image loads

Here's a suggested improvement:

 image.addEventListener(
   "error",
   () => {
+    const error = {
+      type: 'IMAGE_LOAD_ERROR',
+      sampleId,
+      url: source,
+      timestamp: Date.now()
+    };
     console.error(
-      "Failed to load image for sample with id",
-      sampleId,
-      "at url",
-      source
+      'Image load failed:',
+      JSON.stringify(error)
     );
+    // todo: add retry logic here
     image.src = BASE64_BLACK_IMAGE;
   },
   { signal: this.abortController.signal }
 );

Committable suggestion was skipped due to low confidence.


image.src = source;
});
Expand Down Expand Up @@ -124,5 +135,6 @@ export class ImaVidFrameSamples {
this.reverseFrameIndex.clear();
this.samples.clear();
this.storeBufferManager.reset();
this.abortController.abort();
}
}
50 changes: 32 additions & 18 deletions app/packages/looker/src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,8 @@ export const createWorker = (
listeners?: {
[key: string]: ((worker: Worker, args: any) => void)[];
},
dispatchEvent?: DispatchEvent
dispatchEvent?: DispatchEvent,
abortController?: AbortController
Comment on lines +449 to +450
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add null check for abortController parameter

The abortController parameter is optional but used without null checks in the event listeners. This could lead to runtime errors if the function is called without providing an AbortController.

Add a null check at the beginning of the function:

 export const createWorker = (
   listeners?: {
     [key: string]: ((worker: Worker, args: any) => void)[];
   },
   dispatchEvent?: DispatchEvent,
   abortController?: AbortController
 ): Worker => {
+  if (!abortController) {
+    throw new Error('AbortController is required for proper event listener cleanup');
+  }
   let worker: Worker = null;

Committable suggestion was skipped due to low confidence.

): Worker => {
let worker: Worker = null;

Expand All @@ -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",
Expand All @@ -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 }
);
Comment on lines +490 to +500
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving type safety for message handling

While the message handling implementation is good, we could improve type safety by:

  1. Adding type definitions for the message data
  2. Adding runtime type checks for the message structure

Consider applying this improvement:

+interface WorkerMessage {
+  method: string;
+  [key: string]: any;
+}

 worker.addEventListener(
   "message",
-  ({ data: { method, ...args } }) => {
+  ({ data }: MessageEvent<WorkerMessage>) => {
+    const { method, ...args } = data;
+    if (typeof method !== 'string') {
+      console.warn('Received worker message with invalid method type');
+      return;
+    }
     if (!(method in listeners)) {
       return;
     }
     listeners[method].forEach((callback) => callback(worker, args));
   },
   { signal: abortController.signal }
 );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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 }
);
interface WorkerMessage {
method: string;
[key: string]: any;
}
worker.addEventListener(
"message",
({ data }: MessageEvent<WorkerMessage>) => {
const { method, ...args } = data;
if (typeof method !== 'string') {
console.warn('Received worker message with invalid method type');
return;
}
if (!(method in listeners)) {
return;
}
listeners[method].forEach((callback) => callback(worker, args));
},
{ signal: abortController.signal }
);

return worker;
};

Expand Down
20 changes: 16 additions & 4 deletions app/packages/state/src/hooks/useCreateLooker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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);
Expand Down Expand Up @@ -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 }) =>
(
Expand Down Expand Up @@ -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;
},
Expand Down
Loading