BXC-4780 - Handling hung kakadu processes (#28)

* Allow commands to timeout. Read output and error in parallel to waiting, otherwise it can deadlock. Add specialized command exceptions and adjust error handling

* Add special case handling of images that are grayscale alpha but contain rgb data, which was causing kakadu to hang forever

* Trim imageType

* fix tests

* Move trim

---------

Co-authored-by: krwong <krwong@email.unc.edu>

src/main/java/JP2ImageConverter/errors/CommandException.java

@@ -0,0 +1,53 @@
+package JP2ImageConverter.errors;
+
+import java.util.List;
+
+/**
+ * @author bbpennel
+ */
+public class CommandException extends RuntimeException {
+    private static final long serialVersionUID = 1L;
+    private final int exitCode;
+    private final String output;
+    private final List<String> command;
+
+    public CommandException(String message, List<String> command, String output, Throwable cause) {
+        this(message, command, output, -1, cause);
+    }
+
+    public CommandException(String message, List<String> command, String output, int exitCode) {
+        this(message, command, output, exitCode, null);
+    }
+
+    public CommandException(String message, List<String> command, String output, int exitCode, Throwable cause) {
+        super(message, cause);
+        this.exitCode = exitCode;
+        this.output = output;
+        this.command = command;
+    }
+
+    @Override
+    public String getMessage() {
+        var message = super.getMessage()
+                + System.lineSeparator() + "Command: " + String.join(" ", getCommand());
+        if (getExitCode() != -1) {
+            message += System.lineSeparator() + " with exit code: " + getExitCode();
+        }
+        if (getOutput() != null) {
+            message += System.lineSeparator() + " with output: " + getOutput();
+        }
+        return message;
+    }
+
+    public int getExitCode() {
+        return exitCode;
+    }
+
+    public String getOutput() {
+        return output;
+    }
+
+    public List<String> getCommand() {
+        return command;
+    }
+}

src/main/java/JP2ImageConverter/errors/CommandTimeoutException.java

@@ -0,0 +1,14 @@
+package JP2ImageConverter.errors;
+
+import java.util.List;
+
+/**
+ * @author bbpennel
+ */
+public class CommandTimeoutException extends CommandException {
+    private static final long serialVersionUID = 1L;
+
+    public CommandTimeoutException(String message, List<String> command, String output) {
+        super(message, command, output, -1);
+    }
+}

src/main/java/JP2ImageConverter/services/ColorFieldsService.java

@@ -1,5 +1,6 @@
 package JP2ImageConverter.services;
 
+import JP2ImageConverter.errors.CommandException;
 import JP2ImageConverter.util.CommandUtility;
 import com.drew.imaging.ImageMetadataReader;
 import com.drew.imaging.ImageProcessingException;
@@ -180,11 +181,11 @@ public String identifyType(String fileName) {
 
         try {
             colorspace = CommandUtility.executeCommand(command);
-        } catch (Exception e) {
+        } catch (CommandException e) {
             log.warn("Colorspace not identified: {}", e.getMessage());
         }
 
-        return colorspace;
+        return colorspace != null ? colorspace.trim() : null;
     }
 
     /**

src/main/java/JP2ImageConverter/services/ImagePreproccessingService.java

@@ -203,6 +203,21 @@ public String convertRw2(String fileName) throws Exception {
         return temporaryFile;
     }
 
+    /**
+     * Removes the alpha channel from the provided image
+     * @param fileName
+     * @return
+     * @throws Exception
+     */
+    public String removeAlphaChannel(String fileName) throws Exception {
+        String temporaryFile = String.valueOf(prepareTempPath(fileName, ".tif"));
+
+        List<String> command = Arrays.asList(CONVERT, AUTO_ORIENT, "-alpha", "off", fileName, temporaryFile);
+        CommandUtility.executeCommand(command);
+
+        return temporaryFile;
+    }
+
     /**
      * Determine image format and preprocess if needed
      * for non-TIFF image formats: convert to temporary TIFF/PPM before kdu_compress

src/main/java/JP2ImageConverter/services/KakaduService.java

@@ -1,5 +1,6 @@
 package JP2ImageConverter.services;
 
+import JP2ImageConverter.errors.CommandException;
 import JP2ImageConverter.util.CommandUtility;
 import org.apache.commons.io.FilenameUtils;
 import org.slf4j.Logger;
@@ -16,7 +17,9 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
+import static JP2ImageConverter.services.ColorFieldsService.PHOTOMETRIC_INTERPRETATION;
 import static org.slf4j.LoggerFactory.getLogger;
 
 /**
@@ -26,6 +29,9 @@
  */
 public class KakaduService {
     private static final Logger log = getLogger(KakaduService.class);
+    public static final String COLOR_SPACE = "ColorSpace";
+    public static final String COLOR_TYPE = "Type";
+
     private final static Map<String, String> SOURCE_FORMATS = new HashMap<>();
     // accepted file types are listed in sourceFormats below
     static {
@@ -78,7 +84,7 @@ public class KakaduService {
      * @param originalImage the original input image
      * @return colorSpace
      */
-    public String getColorSpace(Map<String, String> preprocessedImageMetadata,
+    public Map<String, String> getColorInfo(Map<String, String> preprocessedImageMetadata,
                                 Map<String, String> originalImageMetadata,
                                 String originalImage) {
         String colorSpace;
@@ -92,13 +98,13 @@ public String getColorSpace(Map<String, String> preprocessedImageMetadata,
             colorSpace = "Gray";
         } else if (preprocessedImageMetadata.get(ColorFieldsService.COLOR_SPACE) != null) {
             colorSpace = preprocessedImageMetadata.get(ColorFieldsService.COLOR_SPACE);
-        } else if (preprocessedImageMetadata.get(ColorFieldsService.PHOTOMETRIC_INTERPRETATION) != null) {
-            colorSpace = preprocessedImageMetadata.get(ColorFieldsService.PHOTOMETRIC_INTERPRETATION);
+        } else if (preprocessedImageMetadata.get(PHOTOMETRIC_INTERPRETATION) != null) {
+            colorSpace = preprocessedImageMetadata.get(PHOTOMETRIC_INTERPRETATION);
         } else if (originalImageMetadata.get(ColorFieldsService.COLOR_SPACE) != null) {
             colorSpace = originalImageMetadata.get(ColorFieldsService.COLOR_SPACE);
-        } else if (originalImageMetadata.get(ColorFieldsService.PHOTOMETRIC_INTERPRETATION) != null &&
-                !originalImageMetadata.get(ColorFieldsService.PHOTOMETRIC_INTERPRETATION).equalsIgnoreCase("ycbcr")) {
-            colorSpace = originalImageMetadata.get(ColorFieldsService.PHOTOMETRIC_INTERPRETATION);
+        } else if (originalImageMetadata.get(PHOTOMETRIC_INTERPRETATION) != null &&
+                !originalImageMetadata.get(PHOTOMETRIC_INTERPRETATION).equalsIgnoreCase("ycbcr")) {
+            colorSpace = originalImageMetadata.get(PHOTOMETRIC_INTERPRETATION);
         } else {
             colorSpace = "sRGB";
         }
@@ -114,7 +120,7 @@ public String getColorSpace(Map<String, String> preprocessedImageMetadata,
             colorSpace = "aToB0";
         }
 
-        return colorSpace;
+        return Map.of(COLOR_SPACE, colorSpace, COLOR_TYPE, imageType == null? "" : imageType);
     }
 
     /**
@@ -194,9 +200,9 @@ public void kduCompress(String sourceFileName, Path outputPath, String sourceFor
             if (!fileName.equals(inputFile)) {
                 preprocessedImageMetadata = extractMetadata(inputFile, "");
             }
-            String colorSpace = getColorSpace(preprocessedImageMetadata, originalImageMetadata, fileName);
-            String orientation = originalImageMetadata.get(ColorFieldsService.ORIENTATION);
-            inputFile = correctInputImage(inputFile, fileName, sourceFormat, colorSpace, orientation, intermediateFiles);
+            var colorInfo = getColorInfo(preprocessedImageMetadata, originalImageMetadata, fileName);
+            var colorSpace = colorInfo.get(COLOR_SPACE);
+            inputFile = correctInputImage(inputFile, fileName, sourceFormat, colorInfo, preprocessedImageMetadata, intermediateFiles);
 
             List<String> command = new ArrayList<>(Arrays.asList(kduCompress, input, inputFile, output, outputFile,
                     clevels, clayers, cprecincts, stiles, corder, orggenplt, orgtparts, cblk, cusesop, cuseeph,
@@ -230,11 +236,11 @@ private void performKakaduCommandWithRecovery(List<String> command, List<String>
         try {
             log.debug("Performing kakadu command: {}", command);
             CommandUtility.executeCommand(command);
-        } catch (Exception e) {
-            var message = e.getMessage();
+        } catch (CommandException e) {
+            var output = e.getOutput();
             if (retry) {
-                if (message.contains("ICC profile") && message.contains("reproduction curve appears to have been truncated")) {
-                    log.warn("Invalid ICC profile, retrying without ICC profile: {}", message);
+                if (output.contains("ICC profile") && output.contains("reproduction curve appears to have been truncated")) {
+                    log.warn("Invalid ICC profile, retrying without ICC profile: {}", e.getMessage());
                     var inputIndex = command.indexOf("-i") + 1;
                     var modifiedTmpPath = imagePreproccessingService.handleIccProfile(command.get(inputIndex));
                     command.set(inputIndex, modifiedTmpPath);
@@ -269,29 +275,39 @@ private String getSourceFormat(String fileName, String sourceFormat) {
      * @param fileName
      * @param sourceFormat
      * @param colorSpace
-     * @param orientation
+     * @param metadata extracted metadata from the source image
      * @param intermediateFiles
      * @return
      * @throws Exception
      */
-    private String correctInputImage(String inputFile, String fileName, String sourceFormat, String colorSpace,
-                                     String orientation, List<String> intermediateFiles) throws Exception {
+    private String correctInputImage(String inputFile, String fileName, String sourceFormat, Map<String, String> colorInfo,
+                                     Map<String, String> metadata, List<String> intermediateFiles) throws Exception {
         var fileBeforeColorConversion = inputFile;
+        var orientation = metadata.get(ColorFieldsService.ORIENTATION);
         // for unusual color spaces (CMYK and YcbCr): convert to temporary TIFF before kduCompress
-        inputFile = imagePreproccessingService.convertColorSpaces(colorSpace, inputFile);
-        if (fileBeforeColorConversion.equals(inputFile)) {
-            // Create a temporary TIFF with the correct orientation if no color space conversion was done
-            // and the orientation is different from the default.
-            var format = sourceFormat != null && !sourceFormat.isEmpty() ?
-                    sourceFormat : SOURCE_FORMATS.get(FilenameUtils.getExtension(fileName));
-            if (orientation != null && format != null && format.equals("tiff")
-                    && !ColorFieldsService.ORIENTATION_DEFAULT.equals(orientation)) {
-                inputFile = imagePreproccessingService.correctOrientation(fileName);
-                intermediateFiles.add(inputFile);
-            }
-        } else {
+        inputFile = imagePreproccessingService.convertColorSpaces(colorInfo.get(COLOR_SPACE), inputFile);
+        if (!fileBeforeColorConversion.equals(inputFile)) {
+            intermediateFiles.add(inputFile);
+            return inputFile;
+        }
+        // Strip alpha channel from grayscale images incorrectly identified as sRGB
+        if ((Objects.equals(metadata.get(PHOTOMETRIC_INTERPRETATION), "RGB")
+                || Objects.equals(metadata.get(ColorFieldsService.COLOR_SPACE), "RGB"))
+                && colorInfo.get(COLOR_TYPE).contains("GrayscaleAlpha")) {
+            inputFile = imagePreproccessingService.removeAlphaChannel(inputFile);
+            intermediateFiles.add(inputFile);
+            return inputFile;
+        }
+        // Create a temporary TIFF with the correct orientation if no color space conversion was done
+        // and the orientation is different from the default.
+        var format = sourceFormat != null && !sourceFormat.isEmpty() ?
+                sourceFormat : SOURCE_FORMATS.get(FilenameUtils.getExtension(fileName));
+        if (orientation != null && format != null && format.equals("tiff")
+                && !ColorFieldsService.ORIENTATION_DEFAULT.equals(orientation)) {
+            inputFile = imagePreproccessingService.correctOrientation(fileName);
             intermediateFiles.add(inputFile);
         }
+
         return inputFile;
     }
 
@@ -322,6 +338,7 @@ public void fileListKduCompress(String fileName, Path outputPath, String sourceF
     public void deleteTinyGrayVoidImages(String outputFile) throws Exception {
         File output = new File(outputFile);
         if (output.length() < 10000 && colorFieldsService.identifyType(outputFile).contains("Gray")) {
+            log.warn("Deleting tiny gray image: {}", outputFile);
             Files.deleteIfExists(Path.of(outputFile));
         }
     }

src/main/java/JP2ImageConverter/util/CommandUtility.java

@@ -1,16 +1,28 @@
 package JP2ImageConverter.util;
 
+import JP2ImageConverter.errors.CommandException;
+import JP2ImageConverter.errors.CommandTimeoutException;
+import org.slf4j.Logger;
+
 import java.io.BufferedReader;
 import java.io.File;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.TimeUnit;
+
+import static org.slf4j.LoggerFactory.getLogger;
 
 /**
  * Utility for executing commands
  * @author krwong
  */
 public class CommandUtility {
+    private static final Logger log = getLogger(CommandUtility.class);
+    private static final int MAX_TIMEOUT_SECONDS = System.getProperty("jp24u.subcommand.timeout") != null ?
+            Integer.parseInt(System.getProperty("jp24u.subcommand.timeout")) : 60 * 5;
 
     private CommandUtility() {
     }
@@ -20,41 +32,78 @@ private CommandUtility() {
      * @param command the command to be executed
      * @return command output
      */
-    public static String executeCommand(List<String> command) throws Exception {
+    public static String executeCommand(List<String> command) {
         StringBuilder output = new StringBuilder();
         try {
             ProcessBuilder builder = new ProcessBuilder(command);
             builder.redirectErrorStream(true);
             Process process = builder.start();
-            InputStream is = process.getInputStream();
-            BufferedReader br = new BufferedReader(new InputStreamReader(is));
-            String line;
 
-            while ((line = br.readLine()) != null) {
-                output.append(line).append(System.lineSeparator());
-            }
-            if (process.waitFor() != 0) {
-                throw new Exception("Command exited with status code " + process.waitFor() + ": " + output);
+            // Use a separate thread to read the output concurrently, to avoid deadlock in case command times out
+            var outputReaderTask = getOutputReaderTask(process.getInputStream(), output);
+
+            waitForProcess(process, command, outputReaderTask, output);
+            // If any errors occurred while reading the output, they will be thrown here
+            outputReaderTask.join();
+            if (process.exitValue() != 0) {
+                throw new CommandException("Command exited with errors", command, output.toString(), process.exitValue());
             }
-        } catch (Exception e) {
-            throw new Exception("Command failed: " + command + System.lineSeparator() + "Output: " + output, e);
+        } catch (InterruptedException | IOException e) {
+            throw new CommandException("Command failed to execute", command, output.toString(), e);
         }
 
         return output.toString();
     }
 
-    public static void executeCommandWriteToFile(List<String> command, String temporaryFile) throws Exception {
+    /**
+     * Run a given command and write the output to a file
+     * @param command
+     * @param temporaryFile
+     */
+    public static void executeCommandWriteToFile(List<String> command, String temporaryFile) {
+        StringBuilder errorOutput = new StringBuilder();
         try {
             ProcessBuilder builder = new ProcessBuilder(command);
-            builder.redirectErrorStream(true);
             builder.redirectOutput(new File(temporaryFile));
             Process process = builder.start();
 
-            if (process.waitFor() != 0) {
-                throw new Exception("Command exited with status code " + process.waitFor());
+            // Use a separate thread to read the errors concurrently, to keep it separate from the main file output
+            var outputReaderTask = getOutputReaderTask(process.getErrorStream(), errorOutput);
+
+            waitForProcess(process, command, outputReaderTask, errorOutput);
+            if (process.exitValue() != 0) {
+                throw new CommandException("Command failed", command, errorOutput.toString(), process.exitValue());
+            }
+        } catch (InterruptedException | IOException e) {
+            throw new CommandException("Command failed to execute", command, errorOutput.toString(), e);
+        }
+    }
+
+    private static CompletableFuture<Void> getOutputReaderTask(InputStream inputStream, StringBuilder output) {
+        return CompletableFuture.runAsync(() -> {
+            try (BufferedReader br = new BufferedReader(new InputStreamReader(inputStream))) {
+                String line;
+                while ((line = br.readLine()) != null) {
+                    output.append(line).append(System.lineSeparator());
+                }
+            } catch (IOException e) {
+                throw new CommandException("Error reading command output", null, output.toString(), e);
+            }
+        });
+    }
+
+    private static void waitForProcess(Process process, List<String> command, CompletableFuture<Void> outputFuture,
+                                       StringBuilder output)
+            throws InterruptedException {
+        log.debug("Waiting for process for {} seconds: {}", MAX_TIMEOUT_SECONDS, command);
+        if (!process.waitFor(MAX_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
+            log.warn("Command timed out, attempting to end process: {}", command);
+            process.destroy();
+            if (outputFuture != null) {
+                outputFuture.join();
             }
-        } catch (Exception e) {
-            throw new Exception("Command failed: " + command, e);
+            throw new CommandTimeoutException("Command timed out after " + MAX_TIMEOUT_SECONDS + " seconds",
+                    command, output.toString());
         }
     }
 }