From 171a35971b00265d7a55d1291e8cf3217afaa69d Mon Sep 17 00:00:00 2001 From: Arnaud Roques Date: Sun, 14 Apr 2024 10:50:33 +0200 Subject: [PATCH] refactor: enhance external process handling using ProcessBuilder --- gradle.properties | 2 +- .../plantuml/dot/AbstractGraphviz.java | 52 ++-- .../plantuml/dot/ProcessRunner.java | 260 ++++-------------- .../plantuml/dot/ProcessState.java | 30 +- .../sourceforge/plantuml/version/Version.java | 2 +- 5 files changed, 79 insertions(+), 267 deletions(-) diff --git a/gradle.properties b/gradle.properties index 708de412d..10742f379 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,4 +1,4 @@ # Warning, "version" should be the same in gradle.properties and Version.java # Any idea anyone how to magically synchronize those :-) ? -version = 1.2024.4 +version = 1.2024.5beta1 org.gradle.workers.max = 3 \ No newline at end of file diff --git a/src/net/sourceforge/plantuml/dot/AbstractGraphviz.java b/src/net/sourceforge/plantuml/dot/AbstractGraphviz.java index 9cb65f9c1..43fd96541 100644 --- a/src/net/sourceforge/plantuml/dot/AbstractGraphviz.java +++ b/src/net/sourceforge/plantuml/dot/AbstractGraphviz.java @@ -61,14 +61,13 @@ abstract class AbstractGraphviz implements Graphviz { private static String findExecutableOnPath(String name) { final String path = System.getenv("PATH"); - if (path != null) { + if (path != null) for (String dirname : path.split(SFile.pathSeparator)) { final File file = new File(dirname, name); - if (file.isFile() && file.canExecute()) { + if (file.isFile() && file.canExecute()) return file.getAbsolutePath(); - } } - } + return null; } @@ -84,12 +83,12 @@ abstract class AbstractGraphviz implements Graphviz { final protected File searchDotExe() { String getenv = GraphvizUtils.getenvGraphvizDot(); - if (findExecutableOnPath() && getenv == null) { + if (findExecutableOnPath() && getenv == null) getenv = findExecutableOnPath(getExeName()); - } - if (getenv == null) { + + if (getenv == null) return specificDotExe(); - } + return new File(getenv); } @@ -100,11 +99,9 @@ abstract class AbstractGraphviz implements Graphviz { final public ProcessState createFile3(OutputStream os) { Objects.requireNonNull(dotString); - if (getExeState() != ExeState.OK) { - // createPngNoGraphviz(os, new - // FileFormatOption(FileFormat.valueOf(type[0].goUpperCase()))); + if (getExeState() != ExeState.OK) throw new IllegalStateException(); - } + final String cmd[] = getCommandLine(); ProcessRunner p = null; ProcessState state = null; @@ -113,9 +110,6 @@ abstract class AbstractGraphviz implements Graphviz { Log.info("DotString size: " + dotString.length()); p = new ProcessRunner(cmd); state = p.run(dotString.getBytes(), os); - // if (state == ProcessState.TERMINATED_OK) { - // result = true; - // } Log.info("Ending process ok"); } catch (Throwable e) { Logme.error(e); @@ -129,15 +123,15 @@ abstract class AbstractGraphviz implements Graphviz { } if (OptionFlags.getInstance().isCheckDotError() && p != null && p.getError().length() > 0) { Log.error("GraphViz error stream : " + p.getError()); - if (OptionFlags.getInstance().isCheckDotError()) { + if (OptionFlags.getInstance().isCheckDotError()) throw new IllegalStateException("Dot error " + p.getError()); - } + } if (OptionFlags.getInstance().isCheckDotError() && p != null && p.getOut().length() > 0) { Log.error("GraphViz out stream : " + p.getOut()); - if (OptionFlags.getInstance().isCheckDotError()) { + if (OptionFlags.getInstance().isCheckDotError()) throw new IllegalStateException("Dot out " + p.getOut()); - } + } return state; } @@ -154,17 +148,17 @@ abstract class AbstractGraphviz implements Graphviz { private String executeCmd(final String cmd[]) { final ProcessRunner p = new ProcessRunner(cmd); final ProcessState state = p.run(null, null); - if (state.differs(ProcessState.TERMINATED_OK())) { + if (state.differs(ProcessState.TERMINATED_OK())) return "?"; - } + final StringBuilder sb = new StringBuilder(); - if (StringUtils.isNotEmpty(p.getOut())) { + if (StringUtils.isNotEmpty(p.getOut())) sb.append(p.getOut()); - } + if (StringUtils.isNotEmpty(p.getError())) { - if (sb.length() > 0) { + if (sb.length() > 0) sb.append(' '); - } + sb.append(p.getError()); } return StringUtils.trin(sb.toString().replace('\n', ' ')); @@ -177,16 +171,16 @@ abstract class AbstractGraphviz implements Graphviz { result[1] = "-n"; result[2] = "10"; result[3] = getDotExe().getAbsolutePath(); - for (int i = 0; i < type.length; i++) { + for (int i = 0; i < type.length; i++) result[i + 4] = "-T" + type[i]; - } + return result; } final String[] result = new String[type.length + 1]; result[0] = getDotExe().getAbsolutePath(); - for (int i = 0; i < type.length; i++) { + for (int i = 0; i < type.length; i++) result[i + 1] = "-T" + type[i]; - } + return result; } diff --git a/src/net/sourceforge/plantuml/dot/ProcessRunner.java b/src/net/sourceforge/plantuml/dot/ProcessRunner.java index 5c56fd568..be1eb195d 100644 --- a/src/net/sourceforge/plantuml/dot/ProcessRunner.java +++ b/src/net/sourceforge/plantuml/dot/ProcessRunner.java @@ -35,15 +35,13 @@ */ package net.sourceforge.plantuml.dot; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.util.concurrent.locks.Lock; -import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.TimeUnit; import net.sourceforge.plantuml.OptionFlags; -import net.sourceforge.plantuml.api.MyRunnable; -import net.sourceforge.plantuml.api.TimeoutExecutor; import net.sourceforge.plantuml.log.Logme; import net.sourceforge.plantuml.security.SFile; @@ -51,209 +49,67 @@ public class ProcessRunner { // ::remove file when __CORE__ private final String[] cmd; - - private String error; - private String out; - - private volatile ProcessState state = ProcessState.INIT(); - private final Lock changeState = new ReentrantLock(); + private String error = ""; + private String out = ""; public ProcessRunner(String[] cmd) { this.cmd = cmd; } - public ProcessState run(byte in[], OutputStream redirection) { + public ProcessState run(byte[] in, OutputStream redirection) { return run(in, redirection, null); } - public ProcessState run(byte in[], OutputStream redirection, SFile dir) { - if (this.state.differs(ProcessState.INIT())) { - throw new IllegalStateException(); - } - this.state = ProcessState.RUNNING(); - final MainThread mainThread = new MainThread(cmd, dir, redirection, in); + public ProcessState run(byte[] in, OutputStream redirection, SFile dir) { try { - // http://steveliles.github.io/invoking_processes_from_java.html + final ProcessBuilder builder = new ProcessBuilder(cmd); + if (dir != null) + builder.directory(dir.conv()); + + builder.redirectErrorStream(true); + + final Process process = builder.start(); + + // Handling input to the process + if (in != null) + try (OutputStream os = process.getOutputStream()) { + os.write(in); + } + + final ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + try (InputStream is = process.getInputStream()) { + final byte[] buffer = new byte[1024]; + int length; + while ((length = is.read(buffer)) != -1) { + outputStream.write(buffer, 0, length); + if (redirection != null) + redirection.write(buffer, 0, length); + } + } + + // Wait for process to terminate final long timeoutMs = OptionFlags.getInstance().getTimeoutMs(); - final boolean done = new TimeoutExecutor(timeoutMs).executeNow(mainThread); - } finally { - changeState.lock(); - try { - if (state.equals(ProcessState.RUNNING())) { - state = ProcessState.TIMEOUT(); - // mainThread.cancel(); - } - } finally { - changeState.unlock(); - } - } - if (state.equals(ProcessState.TERMINATED_OK())) { - assert mainThread != null; - this.error = mainThread.getError(); - this.out = mainThread.getOut(); - } - return state; - } - - class MainThread implements MyRunnable { - - private final String[] cmd; - private final SFile dir; - private final OutputStream redirection; - private final byte[] in; - private volatile Process process; - private volatile ThreadStream errorStream; - private volatile ThreadStream outStream; - - public MainThread(String[] cmd, SFile dir, OutputStream redirection, byte[] in) { - this.cmd = cmd; - this.dir = dir; - this.redirection = redirection; - this.in = in; - } - - public String getOut() { - return outStream.getString(); - } - - public String getError() { - return errorStream.getString(); - } - - public void runJob() throws InterruptedException { - try { - startThreads(); - if (state.equals(ProcessState.RUNNING())) { - final int result = joinInternal(); - } - } finally { - changeState.lock(); - try { - if (state.equals(ProcessState.RUNNING())) { - state = ProcessState.TERMINATED_OK(); - } - } finally { - changeState.unlock(); - } - if (process != null) { - process.destroy(); - close(process.getErrorStream()); - close(process.getOutputStream()); - close(process.getInputStream()); - } + final boolean finished = process.waitFor(timeoutMs, TimeUnit.MILLISECONDS); + outputStream.close(); + if (finished) { + this.out = new String(outputStream.toByteArray(), "UTF-8"); + return ProcessState.TERMINATED_OK(); } - } - - public void cancelJob() { - // The changeState lock is ok - // assert changeState.tryLock(); - // assert state == ProcessState.TIMEOUT; - if (process != null) { - errorStream.cancel(); - outStream.cancel(); - process.destroy(); - // interrupt(); - close(process.getErrorStream()); - close(process.getOutputStream()); - close(process.getInputStream()); + // Process did not finish in time, kill it + process.destroy(); + this.error = "Timeout - kill"; + if (process.waitFor(500, TimeUnit.MILLISECONDS) == false) { + process.destroyForcibly(); + this.error = "Timeout - kill force"; } - } - private void startThreads() { - try { - process = Runtime.getRuntime().exec(cmd, null, dir == null ? null : dir.conv()); - } catch (IOException e) { - Logme.error(e); - changeState.lock(); - try { - state = ProcessState.IO_EXCEPTION1(e); - } finally { - changeState.unlock(); - } - Logme.error(e); - return; - } - errorStream = new ThreadStream(process.getErrorStream(), null); - outStream = new ThreadStream(process.getInputStream(), redirection); - errorStream.start(); - outStream.start(); - if (in != null) { - final OutputStream os = process.getOutputStream(); - try { - try { - os.write(in); - } finally { - os.close(); - } - } catch (IOException e) { - changeState.lock(); - try { - state = ProcessState.IO_EXCEPTION2(e); - } finally { - changeState.unlock(); - } - Logme.error(e); - } - } - } + return ProcessState.TIMEOUT(); - public int joinInternal() throws InterruptedException { - errorStream.join(); - outStream.join(); - final int result = process.waitFor(); - return result; - } - - } - - class ThreadStream extends Thread { - - private volatile InputStream streamToRead; - private volatile OutputStream redirection; - private volatile StringBuffer sb = new StringBuffer(); - - ThreadStream(InputStream streamToRead, OutputStream redirection) { - this.streamToRead = streamToRead; - this.redirection = redirection; - } - - public String getString() { - if (sb == null) { - return ""; - } - return sb.toString(); - } - - public void cancel() { - assert state.equals(ProcessState.TIMEOUT()) || state.equals(ProcessState.RUNNING()) : state; - this.interrupt(); - sb = null; - streamToRead = null; - redirection = null; - // Because of this, some NPE may occurs in run() method, but we do not care - } - - @Override - public void run() { - int read = 0; - try { - while ((read = streamToRead.read()) != -1) { - if (state.equals(ProcessState.TIMEOUT())) { - return; - } - if (redirection == null) { - sb.append((char) read); - } else { - redirection.write(read); - } - } - } catch (Throwable e) { - System.err.println("ProcessRunnerA " + e); - Logme.error(e); - sb.append('\n'); - sb.append(e.toString()); - } + } catch (Throwable e) { + Logme.error(e); + this.error = e.toString(); + return ProcessState.EXCEPTION(e); } } @@ -265,24 +121,4 @@ public class ProcessRunner { return out; } - private void close(InputStream is) { - try { - if (is != null) { - is.close(); - } - } catch (IOException e) { - Logme.error(e); - } - } - - private void close(OutputStream os) { - try { - if (os != null) { - os.close(); - } - } catch (IOException e) { - Logme.error(e); - } - } - } diff --git a/src/net/sourceforge/plantuml/dot/ProcessState.java b/src/net/sourceforge/plantuml/dot/ProcessState.java index 50b57c41c..50792270b 100644 --- a/src/net/sourceforge/plantuml/dot/ProcessState.java +++ b/src/net/sourceforge/plantuml/dot/ProcessState.java @@ -35,42 +35,28 @@ */ package net.sourceforge.plantuml.dot; -import java.io.IOException; - public class ProcessState { // ::remove file when __CORE__ private final String name; - private final IOException cause; + private final Throwable cause; - private ProcessState(String name, IOException cause) { + private ProcessState(String name, Throwable cause) { this.name = name; this.cause = cause; } @Override public String toString() { - if (cause == null) { + if (cause == null) return name; - } + return name + " " + cause.toString(); } - private final static ProcessState INIT = new ProcessState("INIT", null); - private final static ProcessState RUNNING = new ProcessState("RUNNING", null); private final static ProcessState TERMINATED_OK = new ProcessState("TERMINATED_OK", null); private final static ProcessState TIMEOUT = new ProcessState("TIMEOUT", null); - // INIT, RUNNING, TERMINATED_OK, TIMEOUT, IO_EXCEPTION1, IO_EXCEPTION2; - - public static ProcessState INIT() { - return INIT; - } - - public static ProcessState RUNNING() { - return RUNNING; - } - public static ProcessState TERMINATED_OK() { return TERMINATED_OK; } @@ -79,12 +65,8 @@ public class ProcessState { return TIMEOUT; } - public static ProcessState IO_EXCEPTION1(IOException e) { - return new ProcessState("IO_EXCEPTION1", e); - } - - public static ProcessState IO_EXCEPTION2(IOException e) { - return new ProcessState("IO_EXCEPTION2", e); + public static ProcessState EXCEPTION(Throwable e) { + return new ProcessState("EXCEPTION", e); } public boolean differs(ProcessState other) { diff --git a/src/net/sourceforge/plantuml/version/Version.java b/src/net/sourceforge/plantuml/version/Version.java index 37d0ea55d..d55ad232a 100644 --- a/src/net/sourceforge/plantuml/version/Version.java +++ b/src/net/sourceforge/plantuml/version/Version.java @@ -46,7 +46,7 @@ public class Version { // Warning, "version" should be the same in gradle.properties and Version.java // Any idea anyone how to magically synchronize those :-) ? - private static final String version = "1.2024.4"; + private static final String version = "1.2024.5beta1"; public static String versionString() { return version;