Skip to content

Fixes #38495 - Fix memory leak bug in TemplateInvocation file#978

Merged
adamruzicka merged 1 commit into
theforeman:masterfrom
kmalyjur:job-inv-template-memory-leak
Jul 3, 2025
Merged

Fixes #38495 - Fix memory leak bug in TemplateInvocation file#978
adamruzicka merged 1 commit into
theforeman:masterfrom
kmalyjur:job-inv-template-memory-leak

Conversation

@kmalyjur

Copy link
Copy Markdown
Contributor
image

@MariaAga

Copy link
Copy Markdown
Member

Code looks good, Do you remember what actions showed the leak?

@kmalyjur

Copy link
Copy Markdown
Contributor Author

Code looks good, Do you remember what actions showed the leak?

@MariaAga It appeared when I was working on this PR and clicking on the chart or changing the dropdown filter, but I haven't tried any other ways.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes a memory leak in the TemplateInvocation component by replacing a shared intervalId with a useRef-based interval tracker and ensuring all polling intervals are properly cleared.

  • Use useRef (intervalRef) instead of a module-scope intervalId to store the interval handle.
  • Update error handler and effect cleanup to clear intervalRef.current.
  • Ensure cleanup logic runs on unmount and when polling should stop.
Comments suppressed due to low confidence (1)

webpack/JobInvocationDetail/TemplateInvocation.js:87

  • [nitpick] Consider adding unit or integration tests to verify that the polling interval is cleared when the component unmounts or when finished becomes true, to guard against regressions.
}, [finished, autoRefresh, setAPIOptions]);

Comment on lines +80 to +84
clearInterval(intervalRef.current);
}
return () => {
clearInterval(intervalId);
if (intervalRef.current) {
clearInterval(intervalRef.current);

Copilot AI Jun 16, 2025

Copy link

Choose a reason for hiding this comment

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

After calling clearInterval, assign intervalRef.current = null to reset the ref and avoid clearing the same interval repeatedly on future renders.

Copilot uses AI. Check for mistakes.
@MariaAga

Copy link
Copy Markdown
Member

Probably not related to this pr, but when I tried to changing the dropdown filter and then back to all statuses the table is empty (only when autoreload is true)

@kmalyjur
kmalyjur force-pushed the job-inv-template-memory-leak branch from 651da1d to 02cb084 Compare July 2, 2025 07:51
@kmalyjur

kmalyjur commented Jul 2, 2025

Copy link
Copy Markdown
Contributor Author

Do you remember what actions showed the leak?

@MariaAga, the error always appears once after calling /api/job_invocations/id/hosts.

Probably not related to this pr, but when I tried to changing the dropdown filter and then back to all statuses the table is empty (only when autoreload is true)

I don't know how to reproduce it, but I agree that it's probably a separate issue.
Would it be okay to merge this PR, and we can investigate the filter behavior you're seeing later?

@MariaAga MariaAga left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue I described can be solved in another pr yes.
I cant reproduce the issue without the pr, but again, code look good

@adamruzicka
adamruzicka merged commit 66360c0 into theforeman:master Jul 3, 2025
15 of 16 checks passed
@adamruzicka

Copy link
Copy Markdown
Contributor

Thank you @kmalyjur & @MariaAga !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants