gnodet-bot commented on code in PR #29804:
URL: https://github.com/apache/camel/pull/29804#discussion_r4565709614
##########
dsl/camel-jbang/camel-launcher/src/main/java/org/apache/camel/dsl/jbang/launcher/selfupdate/SelfUpdateCommand.java:
##########
@@ -368,10 +373,32 @@ private Integer runInstaller(String targetVersion) {
// pin-state file for this run. Set last so it can never be
dropped by the override block above.
pb.environment().put("CAMEL_INSTALL_SELF_UPDATE", "true");
Process process = pb.start();
- String output = new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8);
- int exit = process.waitFor();
- printer().print(output);
- return exit;
+ // Drain stdout in a separate thread to prevent the subprocess
from blocking on a full pipe buffer in
+ // the gap before waitFor() returns. Then wait with a timeout to
prevent an indefinitely stalled
+ // installer from hanging the CLI.
+ ExecutorService drainer = Executors.newSingleThreadExecutor(r -> {
+ Thread t = new Thread(r, "camel-self-update-drainer");
+ t.setDaemon(true);
+ return t;
+ });
+ Future<String> outputFuture = drainer
+ .submit(() -> new
String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8));
+ try {
+ boolean finished = process.waitFor(5, TimeUnit.MINUTES);
+ if (!finished) {
+ // Kill grandchildren first (curl/wget etc.) before the
shell, so they release the pipe
+
process.descendants().forEach(ProcessHandle::destroyForcibly);
+ process.destroyForcibly();
+ printer().printErr("installer timed out after 5 minutes");
+ return CliExitCode.TIMEOUT;
+ }
+ String output = outputFuture.get(10, TimeUnit.SECONDS);
Review Comment:
`outputFuture.get(10, TimeUnit.SECONDS)` can still throw even after the
process exits cleanly:
- **`TimeoutException`**: a child process (e.g. `curl`, `wget`) that
inherited stdout keeps the pipe open after the shell exits. `readAllBytes()`
blocks until the pipe closes — 10 seconds later this times out, even though the
installer returned exit 0.
- **`ExecutionException`**: a transient I/O error in the drainer thread
propagates here.
Both exceptions escape the `try/finally` block and land in the outer `catch
(Exception e)`, which reports `"failed to run the installer"` and returns 1 — a
false failure when the installer actually succeeded.
**Suggested fix:** wrap `outputFuture.get(...)` in its own try/catch and
treat the timeout as a recoverable condition (the output is best-effort at this
point):
```suggestion
String output;
try {
output = outputFuture.get(10, TimeUnit.SECONDS);
} catch (TimeoutException | ExecutionException e) {
// Output collection timed out or failed — the installer
itself succeeded (exit 0).
// Log a warning but don't misreport the install as
failed.
output = "";
printer().printErr("note: could not collect installer
output", e);
}
```
--
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]
