refactor: enhance external process handling using ProcessBuilder

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

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;
 	}
 

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 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;
-	}
+			final ProcessBuilder builder = new ProcessBuilder(cmd);
+			if (dir != null)
+				builder.directory(dir.conv());
 
-	class MainThread implements MyRunnable {
+			builder.redirectErrorStream(true);
 
-		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;
+			final Process process = builder.start();
 
-		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());
-				}
-			}
-
-		}
-
-		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());
-			}
-		}
-
-		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 {
+			// Handling input to the process
+			if (in != null)
+				try (OutputStream os = process.getOutputStream()) {
 					os.write(in);
-					} finally {
-						os.close();
-					}
-				} catch (IOException e) {
-					changeState.lock();
-					try {
-						state = ProcessState.IO_EXCEPTION2(e);
-					} finally {
-						changeState.unlock();
-					}
-					Logme.error(e);
 				}
+
+			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);
 				}
 			}
 
-		public int joinInternal() throws InterruptedException {
-			errorStream.join();
-			outStream.join();
-			final int result = process.waitFor();
-			return result;
+			// Wait for process to terminate
+			final long timeoutMs = OptionFlags.getInstance().getTimeoutMs();
+			final boolean finished = process.waitFor(timeoutMs, TimeUnit.MILLISECONDS);
+			outputStream.close();
+			if (finished) {
+				this.out = new String(outputStream.toByteArray(), "UTF-8");
+				return ProcessState.TERMINATED_OK();
 			}
 
+			// 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";
 			}
 
-	class ThreadStream extends Thread {
+			return ProcessState.TIMEOUT();
 
-		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());
-			}
+			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);
-		}
-	}
-
 }

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) {

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;