Skip to content

fix: prevent memory leaks from uncleaned uPlot event listeners #9320

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+58 −13
Open

fix: prevent memory leaks from uncleaned uPlot event listeners #9320

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1d1318e
fix: prevent memory leaks from uncleaned uPlot event listeners
ahrefabhi Oct 12, 2025
7ed8e74
fix: prevent memory leak by cleaning up uPlot legend event listeners …
ahrefabhi Oct 14, 2025
62f4603
Merge branch 'main' of https://github.com/SigNoz/signoz into fix/memo…
ahrefabhi Oct 14, 2025
6977467
Merge branch 'main' into fix/memory-utilization
ahrefabhi Oct 16, 2025
b05d683
Merge branch 'main' of https://github.com/SigNoz/signoz into fix/memo…
ahrefabhi Oct 17, 2025
74cafc5
Merge branch 'fix/memory-utilization' of https://github.com/SigNoz/si…
ahrefabhi Oct 17, 2025
876c9d1
fix: pr review changes
ahrefabhi Oct 17, 2025
3cf0300
Merge branch 'main' into fix/memory-utilization
ahrefabhi Oct 17, 2025
8f5624f
Merge branch 'main' of https://github.com/SigNoz/signoz into fix/memo…
ahrefabhi Oct 19, 2025
755bddc
Merge branch 'fix/memory-utilization' of https://github.com/SigNoz/si…
ahrefabhi Oct 19, 2025
b621e9b
chore: removed the cleanup function from uplot component
ahrefabhi Oct 19, 2025
8faa589
Merge branch 'main' into fix/memory-utilization
srikanthccv Oct 20, 2025
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
18 changes: 16 additions & 2 deletions frontend/src/components/Uplot/Uplot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import { dataMatch, optionsUpdateState } from './utils';
// Extended uPlot interface with custom properties
interface ExtendedUPlot extends uPlot {
_legendScrollCleanup?: () => void;
_tooltipCleanup?: () => void;
_legendCleanups?: Array<() => void>;
}

export interface UplotProps {
Expand Down Expand Up @@ -71,12 +73,24 @@ const Uplot = forwardRef<ToggleGraphProps | undefined, UplotProps>(

const destroy = useCallback((chart: uPlot | null) => {
if (chart) {
// Clean up legend scroll event listener
// Clean up all event listeners before destroying
const extendedChart = chart as ExtendedUPlot;

// Clean up all legend event listeners
if (extendedChart._legendCleanups) {
extendedChart._legendCleanups.forEach((cleanup) => cleanup());
}

// Clean up legend scroll event listener
if (extendedChart._legendScrollCleanup) {
extendedChart._legendScrollCleanup();
}

// Clean up tooltip and global document listener
if (extendedChart._tooltipCleanup) {
extendedChart._tooltipCleanup();
}

onDeleteRef.current?.(chart);
chart.destroy();
chartRef.current = null;
Expand All @@ -94,7 +108,7 @@ const Uplot = forwardRef<ToggleGraphProps | undefined, UplotProps>(

// Clean up any remaining tooltips that might be detached
const tooltips = document.querySelectorAll(
'.uplot-tooltip, .tooltip-container',
'.uplot-tooltip, .tooltip-container, .legend-tooltip',
);
tooltips.forEach((tooltip) => {
if (tooltip && tooltip.parentNode) {
Expand Down
63 changes: 61 additions & 2 deletions frontend/src/lib/uPlotLib/getUplotChartOptions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { getYAxisScale } from './utils/getYAxisScale';
interface ExtendedUPlot extends uPlot {
_legendScrollCleanup?: () => void;
_tooltipCleanup?: () => void;
_legendElementCleanup?: Array<() => void>;
}

export interface GetUPlotChartOptions {
Expand Down Expand Up @@ -455,6 +456,9 @@ export const getUPlotChartOptions = ({
],
ready: [
(self): void => {
// Initialize cleanup array for tracking all event listeners
(self as ExtendedUPlot)._legendCleanups = [];

Copy link
Member

Choose a reason for hiding this comment

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

This is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is removed, I'm not removing the cleanup from uplot component is because uplot doesn't directly uses getUPlotChartOptions function which has the hooks.destory cleanup. it get's the options through a prop and let's suppose if someone uses this component directly with own options without using the getUPlotChartOptions function. They there won't be any cleanup.

Anyhow after the cleanup we reset the array, so even if the cleanup runs twice, it'll run on a empty array.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not removing the cleanup from uplot component is because uplot doesn't directly uses getUPlotChartOptions function which has the hooks.destory cleanup. it get's the options through a prop and let's suppose if someone uses this component directly with own options without using the getUPlotChartOptions function. They there won't be any cleanup

That's the main point I was trying to raise with "consolidate this". We should not let it anyone do something that it could lead to memory leak and requires separate cleanup work. Let's please consolidate it. If there is a good reason why it needs to be separate, I want to understand why.

// Add CSS classes to the uPlot container based on legend position
const uplotContainer = self.root;
if (uplotContainer) {
Expand All @@ -473,6 +477,9 @@ export const getUPlotChartOptions = ({
if (legend) {
const legendElement = legend as HTMLElement;

// Initialize cleanup array for legend element listeners
(self as ExtendedUPlot)._legendElementCleanup = [];

// Apply enhanced legend styling
if (enhancedLegend) {
applyEnhancedLegendStyling(
Expand Down Expand Up @@ -639,6 +646,17 @@ export const getUPlotChartOptions = ({
thElement.addEventListener('mouseenter', showTooltip);
thElement.addEventListener('mouseleave', hideTooltip);

// Store cleanup function for tooltip listeners
(self as ExtendedUPlot)._legendElementCleanup?.push(() => {
thElement.removeEventListener('mouseenter', showTooltip);
thElement.removeEventListener('mouseleave', hideTooltip);
// Cleanup any lingering tooltip
if (tooltipElement) {
tooltipElement.remove();
tooltipElement = null;
}
});

// Add click handlers for marker and text separately
const currentMarker = thElement.querySelector('.u-marker');
const textElement = thElement.querySelector('.legend-text');
Expand All @@ -658,7 +676,7 @@ export const getUPlotChartOptions = ({

// Marker click handler - checkbox behavior (toggle individual series)
if (currentMarker) {
currentMarker.addEventListener('click', (e) => {
const markerClickHandler = (e: Event): void => {
e.stopPropagation?.(); // Prevent event bubbling to text handler

if (stackChart) {
Expand All @@ -680,12 +698,19 @@ export const getUPlotChartOptions = ({
return newGraphVisibilityStates;
});
}
};

currentMarker.addEventListener('click', markerClickHandler);

// Store cleanup function for marker click listener
(self as ExtendedUPlot)._legendElementCleanup?.push(() => {
currentMarker.removeEventListener('click', markerClickHandler);
});
}

// Text click handler - show only/show all behavior (existing behavior)
if (textElement) {
textElement.addEventListener('click', (e) => {
const textClickHandler = (e: Event): void => {
e.stopPropagation?.(); // Prevent event bubbling

if (stackChart) {
Expand Down Expand Up @@ -716,13 +741,47 @@ export const getUPlotChartOptions = ({
return newGraphVisibilityStates;
});
}
};

textElement.addEventListener('click', textClickHandler);

// Store cleanup function for text click listener
(self as ExtendedUPlot)._legendElementCleanup?.push(() => {
textElement.removeEventListener('click', textClickHandler);
});
}
}
});
}
},
],
destroy: [
(self): void => {
// Clean up legend scroll listener
if ((self as ExtendedUPlot)._legendScrollCleanup) {
(self as ExtendedUPlot)._legendScrollCleanup?.();
(self as ExtendedUPlot)._legendScrollCleanup = undefined;
}

// Clean up tooltip global listener
if ((self as ExtendedUPlot)._tooltipCleanup) {
(self as ExtendedUPlot)._tooltipCleanup?.();
(self as ExtendedUPlot)._tooltipCleanup = undefined;
}

// Clean up all legend element listeners
if ((self as ExtendedUPlot)._legendElementCleanup) {
(self as ExtendedUPlot)._legendElementCleanup?.forEach((cleanup) => {
cleanup();
});
(self as ExtendedUPlot)._legendElementCleanup = [];
}

// Clean up any remaining tooltips in DOM
const existingTooltips = document.querySelectorAll('.legend-tooltip');
existingTooltips.forEach((tooltip) => tooltip.remove());
},
],
},
series: customSeries
? customSeries(apiResponse?.data?.result || [])
Expand Down
Loading