gnodet-bot commented on code in PR #27927:
URL: https://github.com/apache/camel/pull/27927#discussion_r4289867558
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/process/RestartProcess.java:
##########
@@ -99,40 +93,93 @@ public Integer doCall() throws Exception {
}
if (cmd.isEmpty()) {
- printer().println("Cannot restart: command line not available for
PID " + pid);
- return 1;
+ throw CliException.failure("Cannot restart: command line not
available for PID " + pid);
}
- // stop the process gracefully
- printer().println("Stopping Camel integration: " + displayName + "
(PID: " + pid + ")");
- ph.destroy();
-
- // wait for termination
- CompletableFuture<ProcessHandle> exitFuture =
ph.onExit().toCompletableFuture();
- try {
- exitFuture.get(10, TimeUnit.SECONDS);
- } catch (Exception e) {
- printer().println("Graceful shutdown timed out, force killing...");
- ph.destroyForcibly();
- Thread.sleep(500);
- }
+ stop(ph, displayName);
- // re-launch
+ // re-launch, with its output in a log file that stays (the path is
printed)
ProcessBuilder pb = new ProcessBuilder(cmd);
if (directory != null) {
pb.directory(new File(directory));
}
pb.redirectErrorStream(true);
- Path outputFile = Files.createTempFile("camel-restart-", ".log");
- outputFile.toFile().deleteOnExit();
- pb.redirectOutput(outputFile.toFile());
- Process process = pb.start();
-
- printer().println("Restarted Camel integration: " + displayName + "
(PID: " + process.pid() + ")");
+ Path log = Files.createTempFile(CommandLineHelper.getCamelDir(),
"restart-" + pid + "-", ".log");
Review Comment:
💡 **Minor — restart log accumulation in `~/.camel`:** The previous code used
`deleteOnExit()` so the temp log was cleaned up on JVM exit. The new code
intentionally keeps the log (it's useful for diagnosing a bad restart), but
there's no cleanup mechanism: `restart-<pid>-<random>.log` files will
accumulate in `~/.camel` indefinitely after repeated `camel restart`
invocations.
Consider a lightweight strategy, e.g. delete the log on successful start
(keeping it only on failure), or cap the number of retained restart logs (e.g.
keep the 5 most recent by creation time).
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/process/RestartProcess.java:
##########
@@ -99,40 +93,93 @@ public Integer doCall() throws Exception {
}
if (cmd.isEmpty()) {
- printer().println("Cannot restart: command line not available for
PID " + pid);
- return 1;
+ throw CliException.failure("Cannot restart: command line not
available for PID " + pid);
}
- // stop the process gracefully
- printer().println("Stopping Camel integration: " + displayName + "
(PID: " + pid + ")");
- ph.destroy();
-
- // wait for termination
- CompletableFuture<ProcessHandle> exitFuture =
ph.onExit().toCompletableFuture();
- try {
- exitFuture.get(10, TimeUnit.SECONDS);
- } catch (Exception e) {
- printer().println("Graceful shutdown timed out, force killing...");
- ph.destroyForcibly();
- Thread.sleep(500);
- }
+ stop(ph, displayName);
- // re-launch
+ // re-launch, with its output in a log file that stays (the path is
printed)
ProcessBuilder pb = new ProcessBuilder(cmd);
if (directory != null) {
pb.directory(new File(directory));
}
pb.redirectErrorStream(true);
- Path outputFile = Files.createTempFile("camel-restart-", ".log");
- outputFile.toFile().deleteOnExit();
- pb.redirectOutput(outputFile.toFile());
- Process process = pb.start();
-
- printer().println("Restarted Camel integration: " + displayName + "
(PID: " + process.pid() + ")");
+ Path log = Files.createTempFile(CommandLineHelper.getCamelDir(),
"restart-" + pid + "-", ".log");
+ pb.redirectOutput(log.toFile());
+ Process started = pb.start();
+ awaitStarted(started, displayName, log);
+ printer().println("Restarted Camel integration: " + displayName + "
(PID: " + started.pid() + ")");
+ printer().println("Log: " + log);
return 0;
}
+ /**
+ * Stops the integration like camel stop does, and terminates it when it
does not stop in time: the old one must be
+ * gone before the new one starts.
+ */
+ private void stop(ProcessHandle ph, String displayName) {
+ long pid = ph.pid();
+ printer().println("Stopping Camel integration: " + displayName + "
(PID: " + pid + ")");
+ Path pidFile =
CommandLineHelper.getCamelDir().resolve(Long.toString(pid));
+ if (Files.exists(pidFile)) {
+ PathUtils.deleteFile(pidFile);
+ } else {
+ ph.destroy();
+ }
+ if (StopProcess.awaitExit(pid, System.currentTimeMillis() + timeout)) {
+ return;
+ }
+ printer().printWarn("Camel integration (PID: " + pid + ") did not stop
within "
+ + TimeUtils.printDuration(timeout) + ";
terminating it");
+ ph.destroy();
+ if (StopProcess.awaitExit(pid, System.currentTimeMillis() + 10000)) {
+ return;
+ }
+ ph.destroyForcibly();
+ if (!StopProcess.awaitExit(pid, System.currentTimeMillis() + 5000)) {
+ throw CliException.timeout("Camel integration (PID: " + pid + ")
did not stop, so it is not restarted");
+ }
+ }
+
+ /**
+ * Waits until the new integration has started.
+ *
+ * @throws CliException with exit code 1 when it stopped right away, or
124 when it did not start in time (it is
+ * left running)
+ */
+ private void awaitStarted(Process started, String displayName, Path log)
throws Exception {
+ long deadline = System.currentTimeMillis() + timeout;
+ while (System.currentTimeMillis() < deadline) {
+ if (!started.isAlive()) {
+ printer().printDiagnostic(tail(log, 20));
+ throw CliException.failure("Camel integration " + displayName
+ " stopped right after the restart (exit code "
+ + started.exitValue() + "); the log
is in " + log);
+ }
+ JsonObject status = loadStatus(started.pid());
+ if (status != null && status.get("context") instanceof JsonObject
context
+ && context.getIntegerOrDefault("phase", 0) == STARTED) {
Review Comment:
💡 **Nit — phase check uses exact equality (`== STARTED`):** This works
correctly today since `STARTED = 5` is the terminal "fully started" phase. For
awareness: `Run.java` uses `state < 5` (i.e., not-yet-started) elsewhere. If
future phases are ever inserted above 5 (e.g. `STARTED_WITH_WARNINGS = 6`), the
`== 5` check here would miss them while `>= 5` or a named predicate would not.
No action required unless the phase enum is expected to grow.
--
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]
