From e331de37eee93e9e1b03656f7b1905f42c47c6af Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Mon, 30 Apr 2018 17:49:53 -0400 Subject: [PATCH] improve IOUtils.createTempFile and UserException.BadTmpDir (#4711) * improving error message in UserException.BadTmpDir - updating the commit message to recommend --tmp-dir - propagating the causal exception when relevant - fixes #4709 * simplify and rename tempfile -> createTempFile - use Files.createTempFile instead of existing code using File - schedule temp directories for automatic deletion * removing IOUtils.absolute methods and replacing with calls to Path.normalize --- .../hellbender/exceptions/UserException.java | 14 ++- .../PostprocessGermlineCNVCalls.java | 2 +- .../hellbender/utils/R/RScriptExecutor.java | 4 +- .../hellbender/utils/io/IOUtils.java | 93 ++++--------------- .../hellbender/utils/test/BaseTest.java | 4 +- .../hellbender/utils/io/IOUtilsUnitTest.java | 29 +----- .../runtime/ProcessControllerUnitTest.java | 2 +- 7 files changed, 32 insertions(+), 116 deletions(-) diff --git a/src/main/java/org/broadinstitute/hellbender/exceptions/UserException.java b/src/main/java/org/broadinstitute/hellbender/exceptions/UserException.java index d5040c5cd8e..ace6cc6c243 100644 --- a/src/main/java/org/broadinstitute/hellbender/exceptions/UserException.java +++ b/src/main/java/org/broadinstitute/hellbender/exceptions/UserException.java @@ -2,8 +2,8 @@ import htsjdk.samtools.SAMSequenceDictionary; import htsjdk.tribble.Feature; +import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions; import org.broadinstitute.hellbender.tools.walkers.variantutils.ValidateVariants; -import org.broadinstitute.hellbender.utils.SimpleInterval; import org.broadinstitute.hellbender.utils.help.HelpConstants; import org.broadinstitute.hellbender.utils.read.GATKRead; import org.broadinstitute.hellbender.utils.read.ReadUtils; @@ -11,7 +11,6 @@ import java.io.File; import java.nio.file.Path; import java.util.List; -import java.util.Set; /** *

@@ -183,11 +182,16 @@ public BadInput(String message) { } - public static class BadTmpDir extends UserException { + public static class BadTempDir extends UserException { private static final long serialVersionUID = 0L; - public BadTmpDir(String message) { - super(String.format("Failure working with the tmp directory %s. Override with -Djava.io.tmpdir=X on the command line to a bigger/better file system. Exact error was %s", System.getProperties().get("java.io.tmpdir"), message)); + private static final String MESSAGE_FORMAT_STRING = "Failure working with the tmp directory %s. Try changing the tmp dir with with --" + StandardArgumentDefinitions.TMP_DIR_NAME + " on the command line. Exact error was %s"; + public BadTempDir(String message) { + super(String.format(MESSAGE_FORMAT_STRING, System.getProperties().get("java.io.tmpdir"), message)); + } + + public BadTempDir(String message, Throwable cause) { + super(String.format(MESSAGE_FORMAT_STRING, System.getProperties().get("java.io.tmpdir"), message), cause); } } diff --git a/src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java b/src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java index 2f656186274..2daa9bde1f2 100644 --- a/src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java +++ b/src/main/java/org/broadinstitute/hellbender/tools/copynumber/PostprocessGermlineCNVCalls.java @@ -432,7 +432,7 @@ private void generateSegmentsVCFFileFromAllShards() { logger.info("Generating segments VCF file..."); /* perform segmentation */ - final File pythonScriptOutputPath = IOUtils.tempDir("gcnv-segmented-calls", ""); + final File pythonScriptOutputPath = IOUtils.createTempDir("gcnv-segmented-calls"); final boolean pythonScriptSucceeded = executeSegmentGermlineCNVCallsPythonScript( sampleIndex, contigPloidyCallsPath, sortedCallsShardPaths, sortedModelShardPaths, pythonScriptOutputPath); diff --git a/src/main/java/org/broadinstitute/hellbender/utils/R/RScriptExecutor.java b/src/main/java/org/broadinstitute/hellbender/utils/R/RScriptExecutor.java index f75b303d2f1..6d099ac29ff 100644 --- a/src/main/java/org/broadinstitute/hellbender/utils/R/RScriptExecutor.java +++ b/src/main/java/org/broadinstitute/hellbender/utils/R/RScriptExecutor.java @@ -83,8 +83,8 @@ public RScriptExecutorException getScriptException(final String message) { public boolean exec() { List tempFiles = new ArrayList<>(); try { - File tempLibSourceDir = IOUtils.tempDir("RlibSources.", ""); - File tempLibInstallationDir = IOUtils.tempDir("Rlib.", ""); + File tempLibSourceDir = IOUtils.createTempDir("RlibSources."); + File tempLibInstallationDir = IOUtils.createTempDir("Rlib."); tempFiles.add(tempLibSourceDir); tempFiles.add(tempLibInstallationDir); diff --git a/src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java b/src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java index 82fb63d7653..1db06fb5766 100644 --- a/src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java +++ b/src/main/java/org/broadinstitute/hellbender/utils/io/IOUtils.java @@ -62,38 +62,25 @@ public static boolean isBamFileName(final String inputFileName) { } /** - * Creates a temp directory with the prefix and optional suffix. + * Creates a temp directory with the given prefix. * - * @param prefix Prefix for the directory name. - * @param suffix Optional suffix for the directory name. - * @return The created temporary directory. - */ - public static File tempDir(String prefix, String suffix) { - return tempDir(prefix, suffix, null); - } - - /** - * Creates a temp directory with the prefix and optional suffix. + * The directory and any contents will be automatically deleted at shutdown. + * + * This will not work if the temp dir is not representable as a File. * - * @param prefix Prefix for the directory name. - * @param suffix Optional suffix for the directory name. - * @param tempDirParent Parent directory for the temp directory. + * @param prefix Prefix for the directory name. * @return The created temporary directory. */ - public static File tempDir(String prefix, String suffix, File tempDirParent) { + public static File createTempDir(String prefix) { try { - if (tempDirParent == null) - tempDirParent = FileUtils.getTempDirectory(); - if (!tempDirParent.exists() && !tempDirParent.mkdirs()) - throw new UserException.BadTmpDir("Could not create temp directory: " + tempDirParent); - File temp = File.createTempFile(prefix, suffix, tempDirParent); - if (!temp.delete()) - throw new UserException.BadTmpDir("Could not delete sub file: " + temp.getAbsolutePath()); - if (!temp.mkdir()) - throw new UserException.BadTmpDir("Could not create sub directory: " + temp.getAbsolutePath()); - return absolute(temp); - } catch (IOException e) { - throw new UserException.BadTmpDir(e.getMessage()); + final File tmpDir = Files.createTempDirectory(prefix) + .normalize() + .toFile(); + deleteRecursivelyOnExit(tmpDir); + + return tmpDir; + } catch (final IOException | SecurityException e) { + throw new UserException.BadTempDir(e.getMessage(), e); } } @@ -120,11 +107,11 @@ public static File writeTempFile(String content, String prefix, String suffix) { */ public static File writeTempFile(String content, String prefix, String suffix, File directory) { try { - File tempFile = absolute(File.createTempFile(prefix, suffix, directory)); + File tempFile = File.createTempFile(prefix, suffix, directory).toPath().normalize().toFile(); FileUtils.writeStringToFile(tempFile, content, StandardCharsets.UTF_8); return tempFile; } catch (IOException e) { - throw new UserException.BadTmpDir(e.getMessage()); + throw new UserException.BadTempDir(e.getMessage(), e); } } @@ -157,52 +144,6 @@ else if (file.exists()) return deleted; } - /** - * A mix of getCanonicalFile and getAbsoluteFile that returns the - * absolute path to the file without deferencing symbolic links. - * - * @param file the file. - * @return the absolute path to the file. - */ - public static File absolute(File file) { - return new File(absolutePath(file)); - } - - private static String absolutePath(File file) { - File fileAbs = file.getAbsoluteFile(); - LinkedList names = new LinkedList<>(); - while (fileAbs != null) { - String name = fileAbs.getName(); - fileAbs = fileAbs.getParentFile(); - - if (".".equals(name)) { - /* skip */ - - /* TODO: What do we do for ".."? - } else if (name == "..") { - - CentOS tcsh says use getCanonicalFile: - ~ $ mkdir -p test1/test2 - ~ $ ln -s test1/test2 test3 - ~ $ cd test3/.. - ~/test1 $ - - Mac bash says keep going with getAbsoluteFile: - ~ $ mkdir -p test1/test2 - ~ $ ln -s test1/test2 test3 - ~ $ cd test3/.. - ~ $ - - For now, leave it and let the shell figure it out. - */ - } else { - names.add(0, name); - } - } - - return ("/" + StringUtils.join(names, "/")); - } - /** * Writes an embedded resource to a temp file. * File is not scheduled for deletion and must be cleaned up by the caller. @@ -214,7 +155,7 @@ public static File writeTempResource(Resource resource) { try { temp = File.createTempFile(FilenameUtils.getBaseName(resource.getPath()) + ".", "." + FilenameUtils.getExtension(resource.getPath())); } catch (IOException e) { - throw new UserException.BadTmpDir(e.getMessage()); + throw new UserException.BadTempDir(e.getMessage(), e); } writeResource(resource, temp); return temp; diff --git a/src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java b/src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java index 3b96263b649..a0937ddcf4b 100644 --- a/src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java +++ b/src/main/java/org/broadinstitute/hellbender/utils/test/BaseTest.java @@ -271,9 +271,7 @@ public static Path getSafeNonExistentPath(final String fileNameWithExtension) { * @return an empty directory starting with prefix which will be deleted after the program exits */ public static File createTempDir(final String prefix){ - final File dir = IOUtils.tempDir(prefix, ""); - IOUtils.deleteRecursivelyOnExit(dir); - return dir; + return IOUtils.createTempDir(prefix); } /** diff --git a/src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java b/src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java index 4ce9b52058a..62a71bdfcc2 100644 --- a/src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java +++ b/src/test/java/org/broadinstitute/hellbender/utils/io/IOUtilsUnitTest.java @@ -21,7 +21,7 @@ public final class IOUtilsUnitTest extends GATKBaseTest { @Test public void testTempDir() { - File tempDir = IOUtils.tempDir("Q-Unit-Test", "", new File("queueTempDirToDelete")); + File tempDir = IOUtils.createTempDir("Q-Unit-Test"); Assert.assertTrue(tempDir.exists()); Assert.assertFalse(tempDir.isFile()); Assert.assertTrue(tempDir.isDirectory()); @@ -30,33 +30,6 @@ public void testTempDir() { Assert.assertFalse(tempDir.exists()); } - @Test - public void testAbsolute() { - File dir = IOUtils.absolute(new File("/path/./to/./directory/.")); - Assert.assertEquals(dir, new File("/path/to/directory")); - - dir = IOUtils.absolute(new File("/")); - Assert.assertEquals(dir, new File("/")); - - dir = IOUtils.absolute(new File("/.")); - Assert.assertEquals(dir, new File("/")); - - dir = IOUtils.absolute(new File("/././.")); - Assert.assertEquals(dir, new File("/")); - - dir = IOUtils.absolute(new File("/./directory/.")); - Assert.assertEquals(dir, new File("/directory")); - - dir = IOUtils.absolute(new File("/./directory/./")); - Assert.assertEquals(dir, new File("/directory")); - - dir = IOUtils.absolute(new File("/./directory./")); - Assert.assertEquals(dir, new File("/directory.")); - - dir = IOUtils.absolute(new File("/./.directory/")); - Assert.assertEquals(dir, new File("/.directory")); - } - @Test public void testIsSpecialFile() { Assert.assertTrue(IOUtils.isSpecialFile(new File("/dev"))); diff --git a/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerUnitTest.java b/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerUnitTest.java index 2b3e58b8219..a1e0869b348 100644 --- a/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerUnitTest.java +++ b/src/test/java/org/broadinstitute/hellbender/utils/runtime/ProcessControllerUnitTest.java @@ -415,7 +415,7 @@ private static File writeScript(String contents) { FileUtils.writeStringToFile(file, contents, StandardCharsets.UTF_8); return file; } catch (IOException e) { - throw new UserException.BadTmpDir(e.getMessage()); + throw new UserException.BadTempDir(e.getMessage(), e); } }