Skip to content

fix: add destroy methods to prevent memory leaks in protocol clients #129

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

Merged
kezhenxu94 merged 1 commit into from
Jan 29, 2026
Merged

fix: add destroy methods to prevent memory leaks in protocol clients #129

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
2 changes: 2 additions & 0 deletions src/agent/protocol/Protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,6 @@ export default interface Protocol {
report(): this;

flush(): Promise<any> | null;

destroy?(): void;
}
6 changes: 6 additions & 0 deletions src/agent/protocol/grpc/GrpcProtocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,10 @@ export default class GrpcProtocol implements Protocol {
flush(): Promise<any> | null {
return this.traceReportClient.flush();
}

destroy(): void {
// Clean up both clients to prevent memory leaks
this.heartbeatClient.destroy?.();
this.traceReportClient.destroy?.();
}
}
2 changes: 2 additions & 0 deletions src/agent/protocol/grpc/clients/Client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ export default interface Client {
start(): void;

flush(): Promise<any> | null;

destroy?(): void;
}
10 changes: 10 additions & 0 deletions src/agent/protocol/grpc/clients/HeartbeatClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,14 @@ export default class HeartbeatClient implements Client {
logger.warn('HeartbeatClient does not need flush().');
return null;
}

destroy(): void {
// Clear heartbeat timer to prevent memory leak
if (this.heartbeatTimer) {
clearInterval(this.heartbeatTimer);
this.heartbeatTimer = undefined;
}

Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The gRPC client channel should be closed when destroying the client to properly release network resources. The gRPC library provides a close() method on the client to gracefully shutdown the channel. Consider adding this.managementServiceClient.close() after clearing the heartbeat timer to ensure all network connections are properly terminated.

Suggested change
// Close gRPC client to release underlying network resources
this.managementServiceClient.close();

Copilot uses AI. Check for mistakes.
logger.info('HeartbeatClient destroyed and resources cleaned up');
}
Comment on lines +93 to +101
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The destroy() method should be idempotent to handle cases where it's called multiple times. Consider adding a flag to track if the client has already been destroyed and return early if it's called again. This prevents potential errors from clearing already-cleared resources.

Copilot uses AI. Check for mistakes.
}
37 changes: 35 additions & 2 deletions src/agent/protocol/grpc/clients/TraceReportClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,31 @@ export default class TraceReportClient implements Client {
private readonly reporterClient: TraceSegmentReportServiceClient;
private readonly buffer: Segment[] = [];
private timeout?: NodeJS.Timeout;
private segmentFinishedListener: (segment: Segment) => void;

constructor() {
this.reporterClient = new TraceSegmentReportServiceClient(
config.collectorAddress,
config.secure ? grpc.credentials.createSsl() : grpc.credentials.createInsecure(),
);
emitter.on('segment-finished', (segment) => {

// Store listener reference for cleanup
this.segmentFinishedListener = (segment: Segment) => {
// Limit buffer size to prevent memory leak during network issues
if (this.buffer.length >= config.maxBufferSize) {
logger.warn(
`Trace buffer reached maximum size (${config.maxBufferSize}). ` +
`Discarding oldest segment to prevent memory leak. ` +
`This may indicate network connectivity issues with the collector.`
);
this.buffer.shift(); // Remove oldest segment
}

this.buffer.push(segment);
this.timeout?.ref();
});
};

emitter.on('segment-finished', this.segmentFinishedListener);
}

get isConnected(): boolean {
Expand Down Expand Up @@ -107,4 +122,22 @@ export default class TraceReportClient implements Client {
this.reportFunction(resolve);
});
}

destroy(): void {
// Clean up event listener to prevent memory leak
if (this.segmentFinishedListener) {
emitter.off('segment-finished', this.segmentFinishedListener);
}
Comment on lines +128 to +130
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The null check if (this.segmentFinishedListener) is unnecessary since the listener is always assigned in the constructor and never set to null or undefined elsewhere in the code. This check can be safely removed for cleaner code.

Suggested change
if (this.segmentFinishedListener) {
emitter.off('segment-finished', this.segmentFinishedListener);
}
emitter.off('segment-finished', this.segmentFinishedListener);

Copilot uses AI. Check for mistakes.

// Clear timeout
if (this.timeout) {
clearTimeout(this.timeout);
this.timeout = undefined;
}

// Clear buffer
this.buffer.length = 0;
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

There's a potential race condition if destroy() is called while segments are being reported. The buffer could be cleared at line 139 while reportFunction() is iterating over it (lines 91-99), potentially causing data loss or unexpected behavior. Consider either waiting for in-flight reports to complete before cleanup, or adding synchronization to prevent concurrent access to the buffer.

Copilot uses AI. Check for mistakes.

logger.info('TraceReportClient destroyed and resources cleaned up');
}
Comment on lines +126 to +142
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The gRPC client channels should be closed when destroying the client to properly release network resources. The gRPC library provides a close() method on the client to gracefully shutdown the channel. Consider adding this.reporterClient.close() after clearing the timeout and before clearing the buffer to ensure all network connections are properly terminated.

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +142
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

There's a potential race condition if destroy() is called while reportFunction() is executing. The timeout could be cleared while the report is in progress, but then reportFunction() might schedule a new timeout at line 106 after destroy has completed. Consider adding a flag to track if the client is being destroyed and check it before scheduling new timeouts in reportFunction().

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +142
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The destroy() method should be idempotent to handle cases where it's called multiple times. Consider adding a flag to track if the client has already been destroyed and return early if it's called again. This prevents potential errors from clearing already-cleared resources and removes event listeners multiple times.

Copilot uses AI. Check for mistakes.
}
14 changes: 14 additions & 0 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,20 @@ class Agent {
});
});
}

destroy(): void {
if (this.protocol === null) {
logger.warn('Trying to destroy() SkyWalking agent which is not started.');
return;
}

logger.info('Destroying SkyWalking agent and cleaning up resources');

// Clean up protocol resources
this.protocol.destroy?.();
this.protocol = null;
Comment on lines +85 to +86
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

Consider calling flush() before destroying the protocol to ensure any pending segments are reported to the backend before cleanup. This would prevent potential data loss of in-flight or buffered trace segments during shutdown. The flush could be made asynchronous to allow graceful completion of pending reports.

Copilot uses AI. Check for mistakes.
this.started = false;
}
}

export default new Agent();
Expand Down
Loading