From 27988110d76e2b06de7cd477de549159db3db066 Mon Sep 17 00:00:00 2001 From: DerDavidBohl Date: Wed, 10 Dec 2025 12:55:09 +0100 Subject: [PATCH] Refactor ProcessRunner to remove process tracking and cleanup logic, simplifying command execution --- .../utility/process/ProcessRunner.java | 158 ++++-------------- 1 file changed, 35 insertions(+), 123 deletions(-) diff --git a/backend/src/main/java/org/davidbohl/dirigent/utility/process/ProcessRunner.java b/backend/src/main/java/org/davidbohl/dirigent/utility/process/ProcessRunner.java index 5ddc057..b0d710e 100644 --- a/backend/src/main/java/org/davidbohl/dirigent/utility/process/ProcessRunner.java +++ b/backend/src/main/java/org/davidbohl/dirigent/utility/process/ProcessRunner.java @@ -8,82 +8,16 @@ import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import org.springframework.stereotype.Component; -import jakarta.annotation.PreDestroy; import lombok.extern.slf4j.Slf4j; @Component @Slf4j public class ProcessRunner { - // Track active processes to ensure cleanup - private final Map activeProcesses = new ConcurrentHashMap<>(); - - // Background thread to clean up any stale processes - private final ScheduledExecutorService cleanupExecutor = Executors.newSingleThreadScheduledExecutor(r -> { - Thread t = new Thread(r, "process-cleanup-thread"); - t.setDaemon(true); - return t; - }); - - public ProcessRunner() { - // Start periodic cleanup task to catch any missed processes - cleanupExecutor.scheduleAtFixedRate(this::cleanupStaleProcesses, 10, 10, TimeUnit.SECONDS); - log.info("ProcessRunner initialized with background cleanup thread"); - } - - @PreDestroy - public void shutdown() { - log.info("Shutting down ProcessRunner and cleaning up {} remaining processes", activeProcesses.size()); - cleanupStaleProcesses(); - cleanupExecutor.shutdown(); - try { - if (!cleanupExecutor.awaitTermination(5, TimeUnit.SECONDS)) { - cleanupExecutor.shutdownNow(); - } - } catch (InterruptedException e) { - cleanupExecutor.shutdownNow(); - Thread.currentThread().interrupt(); - } - } - - private void cleanupStaleProcesses() { - long now = System.currentTimeMillis(); - activeProcesses.entrySet().removeIf(entry -> { - Process process = entry.getKey(); - long startTime = entry.getValue(); - - // Check if process is still alive - if (!process.isAlive()) { - log.debug("Removing dead process from tracking (age: {}s)", (now - startTime) / 1000); - return true; - } - - // If process has been running for more than 2 minutes, kill it - if ((now - startTime) > 120000) { - log.warn("Killing stale process (age: {}s)", (now - startTime) / 1000); - process.destroyForcibly(); - try { - process.waitFor(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - return true; - } - return false; - }); - - if (!activeProcesses.isEmpty()) { - log.debug("Active processes being tracked: {}", activeProcesses.size()); - } - } - public ProcessResult executeCommand(List commandParts, long timeoutMs, Map env) throws IOException { return executeInternal(commandParts, new File(System.getProperty("user.dir")), timeoutMs, env); @@ -114,14 +48,13 @@ public class ProcessRunner { Map finalEnv = new HashMap<>(); finalEnv.putAll(System.getenv()); - - if(env != null && !env.isEmpty()) + if(env != null && !env.isEmpty()) { finalEnv.putAll(env); + } ProcessBuilder processBuilder = new ProcessBuilder(commandParts); processBuilder.directory(workingDirectory); processBuilder.environment().putAll(finalEnv); - processBuilder.redirectErrorStream(false); log.debug("Running command <{}> in directory {}", String.join(" ", commandParts), workingDirectory); @@ -133,56 +66,28 @@ public class ProcessRunner { try { process = processBuilder.start(); - // Track this process immediately - activeProcesses.put(process, System.currentTimeMillis()); - - // Read stdout in separate thread - Thread stdoutReader = new Thread(() -> { - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(process.getInputStream(), StandardCharsets.UTF_8))) { - String line; - while ((line = reader.readLine()) != null) { - stdout.append(line).append("\n"); - } - } catch (IOException e) { - log.debug("Error reading stdout", e); - } - }, "stdout-reader"); - stdoutReader.setDaemon(true); - stdoutReader.start(); + // Read stdout + Thread stdoutReader = readStream(process.getInputStream(), stdout); + // Read stderr + Thread stderrReader = readStream(process.getErrorStream(), stderr); - // Read stderr in separate thread - Thread stderrReader = new Thread(() -> { - try (BufferedReader reader = new BufferedReader( - new InputStreamReader(process.getErrorStream(), StandardCharsets.UTF_8))) { - String line; - while ((line = reader.readLine()) != null) { - stderr.append(line).append("\n"); - } - } catch (IOException e) { - log.debug("Error reading stderr", e); - } - }, "stderr-reader"); - stderrReader.setDaemon(true); - stderrReader.start(); - - // Wait for process with timeout + // Wait for process to complete (1 minute timeout) boolean finished = process.waitFor(1, TimeUnit.MINUTES); if (!finished) { - log.warn("Process timed out, killing it: {}", String.join(" ", commandParts)); + log.warn("Process timed out: {}", String.join(" ", commandParts)); process.destroyForcibly(); process.waitFor(5, TimeUnit.SECONDS); } - // Wait for reader threads to finish + // Wait for output streams to finish reading stdoutReader.join(2000); stderrReader.join(2000); exitCode = process.exitValue(); } catch (InterruptedException e) { - log.warn("Process got interrupted: {}", String.join(" ", commandParts), e); + log.warn("Process interrupted: {}", String.join(" ", commandParts), e); if (process != null && process.isAlive()) { process.destroyForcibly(); try { @@ -193,30 +98,37 @@ public class ProcessRunner { } Thread.currentThread().interrupt(); } finally { - // CRITICAL: Always remove from tracking and ensure process is dead - if (process != null) { - activeProcesses.remove(process); - - // Double-check the process is actually dead - if (process.isAlive()) { - log.warn("Process still alive in finally block, force killing"); - process.destroyForcibly(); - try { - process.waitFor(5, TimeUnit.SECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } + // Ensure process is terminated + if (process != null && process.isAlive()) { + log.warn("Force killing remaining process"); + process.destroyForcibly(); + try { + process.waitFor(5, TimeUnit.SECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); } } } - String stdoutString = stdout.toString().trim(); - String stderrString = stderr.toString().trim(); + log.debug("Finished command <{}> with exit code {}", String.join(" ", commandParts), exitCode); - log.debug("Finished command <{}>\nExit code: <{}>\nstdout: {}\nstderr: {}", - String.join(" ", commandParts), exitCode, stdoutString, stderrString); + return new ProcessResult(exitCode, stdout.toString().trim(), stderr.toString().trim()); + } - return new ProcessResult(exitCode, stdoutString, stderrString); + private Thread readStream(java.io.InputStream inputStream, StringBuilder output) { + Thread reader = new Thread(() -> { + try (BufferedReader br = new BufferedReader(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) { + String line; + while ((line = br.readLine()) != null) { + output.append(line).append("\n"); + } + } catch (IOException e) { + log.debug("Error reading stream", e); + } + }); + reader.setDaemon(true); + reader.start(); + return reader; } }