bringhurst commented on code in PR #1717:
URL: https://github.com/apache/samza/pull/1717#discussion_r2198110234
##########
samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java:
##########
@@ -55,6 +55,15 @@ private List<String> getAllCommandOutput(String[] cmdArray)
throws IOException {
}
}
processReader.close();
+
+ // Wait for the process to complete to prevent resource leak
+ try {
+ executable.waitFor();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IOException("Interrupted while waiting for command to
complete", e);
+ }
Review Comment:
It might be good to add:
```
} finally {
executable.destroy();
}
```
here just to make sure.
##########
samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java:
##########
@@ -55,6 +55,15 @@ private List<String> getAllCommandOutput(String[] cmdArray)
throws IOException {
}
}
processReader.close();
+
+ // Wait for the process to complete to prevent resource leak
+ try {
+ executable.waitFor();
+ } catch (InterruptedException e) {
Review Comment:
Before we re-interrupt, we should probably try to kill it off. e.g.
```
} catch (InterruptedException e) {
executable.destroy();
Thread.currentThread().interrupt();
throw new IOException("Interrupted while waiting for command to
complete", e);
}
```
##########
samza-core/src/main/java/org/apache/samza/container/host/PosixCommandBasedStatisticsGetter.java:
##########
@@ -55,6 +55,15 @@ private List<String> getAllCommandOutput(String[] cmdArray)
throws IOException {
}
}
processReader.close();
Review Comment:
Instead of explicitly calling close, this should probably use
try-with-resources instead. Also, since we're already changing this, any
thoughts on draining the stderr buffer here too? Although (very) unlikely, if
the process spams stderr and fills the pipe, it'll deadlock on the wait until
something drains it off. e.g.
```
try (BufferedReader processReader = new BufferedReader(new
InputStreamReader(executable.getInputStream()));
BufferedReader errorReader = new BufferedReader(new
InputStreamReader(executable.getErrorStream()))) {
String line;
while ((line = processReader.readLine()) != null) {
if (!line.isEmpty()) {
psOutput.add(line);
}
}
while ((line = errorReader.readLine()) != null) {
if (!line.isEmpty()) {
log.error("stderr while running {}: {}", cmdArray, line);
}
}
}
```
--
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]
