cleanup event handlers in looker2d #4988
Conversation
WalkthroughThe pull request introduces several modifications across multiple components, primarily focusing on enhancing event listener management and resource cleanup. Key changes include the addition of Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
sashankaryal
force-pushed
the
fix/event-handler-memory-leak
branch
from
f5b9c94 to
df695cc
Compare
benjaminpkane
force-pushed
the
fix/event-handler-memory-leak
branch
from
df695cc to
de40602
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
app/packages/state/src/hooks/useCreateLooker.ts (2)
72-77: Simplify the cleanup function.The cleanup function has an unnecessary return statement that can be simplified.
useEffect(() => { return () => { - // sending abort signal to clean up all event handlers - return abortControllerRef.current.abort(); + // Clean up all event handlers + abortControllerRef.current.abort(); }; }, []);
Line range hint
81-290: Consider splitting the create function for better maintainability.The create function handles multiple responsibilities including:
- Determining the looker type
- Configuring different looker types
- Setting up event listeners
Consider splitting this into smaller, more focused functions for better maintainability and testing.
Some suggestions:
- Extract looker type determination into a separate function
- Create separate configuration factories for each looker type
- Consider using a builder pattern for looker configuration
Example structure:
const determineLookerType = (sample: Sample, urls: Urls) => { // Logic to determine looker type }; const createLookerConfig = (type: LookerType, sample: Sample, options: Options) => { // Type-specific configuration logic }; const create = useRecoilCallback( ({ snapshot }) => (params) => { const lookerType = determineLookerType(params.sample, params.urls); const config = createLookerConfig(lookerType, params.sample, params); const looker = new lookerType(params.sample, config, options); setupEventListeners(looker); return looker; } );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/packages/core/src/components/Modal/Modal.tsx(1 hunks)app/packages/looker/src/lookers/abstract.ts(5 hunks)app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts(3 hunks)app/packages/looker/src/util.ts(3 hunks)app/packages/state/src/hooks/useCreateLooker.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/core/src/components/Modal/Modal.tsx (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/lookers/abstract.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/looker/src/util.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/state/src/hooks/useCreateLooker.ts (1)
Pattern **/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (9)
app/packages/looker/src/lookers/imavid/ima-vid-frame-samples.ts (3)
25-26: LGTM: Well-implemented AbortController addition
The addition of a private readonly AbortController is a good practice for managing the lifecycle of async operations and event listeners.
Also applies to: 29-29
71-92: Consider enhancing error handling and addressing TODO
While the error handling is improved, consider:
- Using a structured error logging approach
- Implementing proper error recovery beyond using a black image
- Adding error metrics/monitoring for this edge case
Would you like me to propose a more robust error handling implementation or help create an issue to track the TODO?
138-138: LGTM: Proper cleanup implementation
The addition of abortController.abort() ensures proper cleanup of pending operations during reset.
app/packages/core/src/components/Modal/Modal.tsx (1)
Line range hint 208-211: Verify event listener management.
The event listener is added in onLookerSet but there's no protection against multiple additions of the same listener.
Let's verify the event listener management pattern in other files:
✅ Verification successful
Event listener management is properly handled
After reviewing the codebase, the event listener management is implemented correctly in the Modal component. The code shows:
- The "close" event listener is added in a controlled way within the component:
looker.addEventListener("close", modalCloseHandler);- The cleanup is properly handled with
removeEventListenerin the cleanup function:
activeLookerRef.current?.removeEventListener("close", modalCloseHandler);This pattern is consistent with React's best practices for event listener management, and similar cleanup patterns are used throughout other modal-related components in the codebase. The concern about multiple additions of the same listener is unfounded as the code follows the standard React lifecycle pattern where listeners are properly cleaned up.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar event listener patterns in looker-related files
rg -A 5 "addEventListener.*close" app/packages/looker/
Length of output: 56
Script:
#!/bin/bash
# Let's search for event listener patterns in the Modal component and related files
rg -A 5 "addEventListener" app/packages/core/src/components/Modal/
# Also search for any cleanup or removal of event listeners
rg -A 5 "removeEventListener" app/packages/core/src/components/Modal/
# Let's also check the specific onLookerSet implementation
ast-grep --pattern 'function onLookerSet($$$) { $$$ }'
Length of output: 10722
app/packages/state/src/hooks/useCreateLooker.ts (1)
259-265: 🛠️ Refactor suggestion
Consider enhancing error context.
While the error handling is functional, consider adding more context about which looker instance failed.
looker.addEventListener(
"error",
(event) => {
- handleError(event.error);
+ handleError({
+ error: event.error,
+ context: {
+ sampleId: sample._id,
+ lookerType: looker.constructor.name
+ }
+ });
},
{ signal: abortControllerRef.current.signal }
);Likely invalid or redundant comment.
app/packages/looker/src/util.ts (1)
460-479: LGTM! Improved error handling with proper cleanup
The error handling has been improved by:
- Using dedicated event listeners for both error and message events
- Properly integrating with
AbortControllerfor cleanup - Maintaining consistent error handling logic
app/packages/looker/src/lookers/abstract.ts (3)
111-112: Appropriate use of AbortController for managing async operations
The addition of the abortController as a private readonly member and its initialization in the constructor is a correct approach for managing cancellation of asynchronous operations within the AbstractLooker instance.
Also applies to: 118-118
395-407: Integrating AbortController with event listeners correctly
Modifying addEventListener to include signal: this.abortController.signal ensures that event listeners are properly cleaned up when this.abortController.abort() is called. This aligns with best practices for managing event listeners in modern JavaScript and TypeScript applications.
511-511: Proper cleanup of resources in the detach method
Calling this.abortController.abort(); in the detach method effectively cancels any ongoing operations and removes associated event listeners, aiding in preventing memory leaks and ensuring that resources are properly released when the instance is detached.
| 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 } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing image loading error recovery
The current implementation could be improved by:
- Adding retry logic for transient failures
- Using structured logging with error categorization
- 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.
| activeLookerRef.current?.removeEventListener( | ||
| "close", | ||
| modalCloseHandler | ||
| ); |
There was a problem hiding this comment.
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.
| activeLookerRef.current?.removeEventListener( | |
| "close", | |
| modalCloseHandler | |
| ); | |
| if (activeLookerRef.current) { | |
| activeLookerRef.current.removeEventListener( | |
| "close", | |
| modalCloseHandler | |
| ); | |
| } |
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.
| dispatchEvent?: DispatchEvent, | ||
| abortController?: AbortController |
There was a problem hiding this comment.
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.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 } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety for message handling
While the message handling implementation is good, we could improve type safety by:
- Adding type definitions for the message data
- 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.
| 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 (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 | ||
| ) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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.
| const labelsWorker = getLabelsWorker( | ||
| (event, detail) => this.dispatchEvent(event, detail), | ||
| this.abortController |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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
AbstractLookerinstance. - Implement a messaging protocol within workers to handle abort signals specific to each instance.
- Avoid passing the instance-specific
abortControllerwhen initializing shared workers.
Would you like assistance in refactoring the worker management to address this issue?
|
Going to merge this. Will follow up with the rest of the 2d changes |
incomplete
Summary by CodeRabbit
New Features
Bug Fixes
Documentation