Copilot commented on code in PR #17212:
URL: https://github.com/apache/iotdb/pull/17212#discussion_r2844123515
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/system/SystemMetrics.java:
##########
@@ -240,6 +242,13 @@ private void updateLinuxSystemMemInfo() {
}
} catch (IOException e) {
logger.debug("Failed to get memory, because ", e);
+ } catch (InterruptedException e) {
+ logger.debug("Interrupted while waiting for memory command", e);
+ Thread.currentThread().interrupt();
+ } finally {
+ if (process != null) {
Review Comment:
Calling destroyForcibly() unconditionally in the finally block can forcibly
terminate a process that has already completed successfully via waitFor(). This
is problematic because:
1. After waitFor() returns successfully, the process has already exited
normally, and destroyForcibly() should not be needed
2. destroyForcibly() may interfere with proper cleanup of process resources
on some platforms
3. This pattern goes against Java best practices for process management
The fix should only call destroyForcibly() when the process hasn't completed
normally (i.e., when an exception occurs before or during waitFor()). Consider
adding a boolean flag to track whether waitFor() completed successfully, or
only destroy the process in the catch blocks.
```suggestion
if (process != null && process.isAlive()) {
```
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/net/LinuxNetMetricManager.java:
##########
@@ -228,9 +229,17 @@ private void updateNetStatus() {
result.append(line);
}
}
+ process.waitFor();
this.connectionNum = Integer.parseInt(result.toString().trim());
} catch (IOException e) {
LOGGER.error("Failed to get socket num", e);
Review Comment:
The Integer.parseInt() call can throw NumberFormatException if the command
output is not a valid integer, but this exception is not caught. If the
external command returns unexpected output, this will cause an unhandled
exception that could disrupt metrics collection. Consider adding a catch block
for NumberFormatException to handle malformed output gracefully, similar to how
IOException is handled.
```suggestion
LOGGER.error("Failed to get socket num", e);
} catch (NumberFormatException e) {
LOGGER.error(
"Failed to parse socket num from command output: '{}'",
e.getMessage());
```
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/metricsets/net/LinuxNetMetricManager.java:
##########
@@ -228,9 +229,17 @@ private void updateNetStatus() {
result.append(line);
}
}
+ process.waitFor();
this.connectionNum = Integer.parseInt(result.toString().trim());
} catch (IOException e) {
LOGGER.error("Failed to get socket num", e);
+ } catch (InterruptedException e) {
+ LOGGER.error("Interrupted while waiting for socket num command", e);
+ Thread.currentThread().interrupt();
+ } finally {
+ if (process != null) {
+ process.destroyForcibly();
+ }
Review Comment:
Calling destroyForcibly() unconditionally in the finally block can forcibly
terminate a process that has already completed successfully via waitFor(). This
is problematic because:
1. After waitFor() returns successfully, the process has already exited
normally, and destroyForcibly() should not be needed
2. destroyForcibly() may interfere with proper cleanup of process resources
on some platforms
3. This pattern goes against Java best practices for process management
The fix should only call destroyForcibly() when the process hasn't completed
normally (i.e., when an exception occurs before or during waitFor()). Consider
adding a boolean flag to track whether waitFor() completed successfully, or
only destroy the process in the catch blocks.
--
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]
