Copilot commented on code in PR #129:
URL: https://github.com/apache/skywalking-nodejs/pull/129#discussion_r2734759815
##########
src/agent/protocol/grpc/clients/TraceReportClient.ts:
##########
@@ -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);
+ }
+
+ // Clear timeout
+ if (this.timeout) {
+ clearTimeout(this.timeout);
+ this.timeout = undefined;
+ }
+
+ // Clear buffer
+ this.buffer.length = 0;
+
+ logger.info('TraceReportClient destroyed and resources cleaned up');
+ }
Review Comment:
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.
##########
src/agent/protocol/grpc/clients/TraceReportClient.ts:
##########
@@ -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);
+ }
+
+ // Clear timeout
+ if (this.timeout) {
+ clearTimeout(this.timeout);
+ this.timeout = undefined;
+ }
+
+ // Clear buffer
+ this.buffer.length = 0;
+
+ logger.info('TraceReportClient destroyed and resources cleaned up');
+ }
Review Comment:
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()`.
##########
src/agent/protocol/grpc/clients/HeartbeatClient.ts:
##########
@@ -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;
+ }
+
+ logger.info('HeartbeatClient destroyed and resources cleaned up');
+ }
Review Comment:
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.
##########
src/index.ts:
##########
@@ -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;
Review Comment:
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.
##########
src/agent/protocol/grpc/clients/TraceReportClient.ts:
##########
@@ -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);
+ }
Review Comment:
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.
```suggestion
emitter.off('segment-finished', this.segmentFinishedListener);
```
##########
src/agent/protocol/grpc/clients/TraceReportClient.ts:
##########
@@ -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);
+ }
+
+ // Clear timeout
+ if (this.timeout) {
+ clearTimeout(this.timeout);
+ this.timeout = undefined;
+ }
+
+ // Clear buffer
+ this.buffer.length = 0;
Review Comment:
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.
##########
src/agent/protocol/grpc/clients/TraceReportClient.ts:
##########
@@ -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);
+ }
+
+ // Clear timeout
+ if (this.timeout) {
+ clearTimeout(this.timeout);
+ this.timeout = undefined;
+ }
+
+ // Clear buffer
+ this.buffer.length = 0;
+
+ logger.info('TraceReportClient destroyed and resources cleaned up');
+ }
Review Comment:
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.
##########
src/agent/protocol/grpc/clients/HeartbeatClient.ts:
##########
@@ -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;
+ }
+
Review Comment:
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.
```suggestion
// Close gRPC client to release underlying network resources
this.managementServiceClient.close();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
