From 2e8197a2c712fad704b60e345abef5881fe215c8 Mon Sep 17 00:00:00 2001 From: Matt Sicker Date: Fri, 14 Dec 2018 10:33:18 -0600 Subject: [PATCH] Complete refactoring of Util.delete methods This removes duplicated tests from UtilTest that were added in PathRemoverTest. This also ports the sleep/gc strategy for use by default to preserve backward compatibility. Signed-off-by: Matt Sicker --- core/src/main/java/hudson/Util.java | 234 +------------- .../java/jenkins/util/io/PathRemover.java | 94 +++++- core/src/test/java/hudson/UtilTest.java | 294 ------------------ .../java/jenkins/util/io/PathRemoverTest.java | 20 +- 4 files changed, 107 insertions(+), 535 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 8007200a62..1f266746aa 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -23,14 +23,13 @@ */ package hudson; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; - import hudson.model.TaskListener; import jenkins.util.MemoryReductionUtil; import hudson.util.QuotedStringTokenizer; import hudson.util.VariableResolver; import jenkins.util.SystemProperties; +import jenkins.util.io.PathRemover; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.IOUtils; import org.apache.commons.io.output.NullOutputStream; @@ -67,7 +66,6 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.BasicFileAttributes; -import java.nio.file.attribute.PosixFileAttributes; import java.nio.file.attribute.DosFileAttributes; import java.nio.file.attribute.PosixFilePermissions; import java.security.DigestInputStream; @@ -247,16 +245,7 @@ public class Util { * if the operation fails. */ public static void deleteContentsRecursive(@Nonnull File file) throws IOException { - for( int numberOfAttempts=1 ; ; numberOfAttempts++ ) { - try { - tryOnceDeleteContentsRecursive(file); - break; // success - } catch (IOException ex) { - boolean threadWasInterrupted = pauseBetweenDeletes(numberOfAttempts); - if( numberOfAttempts>= DELETION_MAX || threadWasInterrupted) - throw new IOException(deleteFailExceptionMessage(file, numberOfAttempts, threadWasInterrupted), ex); - } - } + newPathRemover().forceRemoveDirectoryContents(fileToPath(file)); } /** @@ -267,88 +256,7 @@ public class Util { * @throws IOException if it exists but could not be successfully deleted */ public static void deleteFile(@Nonnull File f) throws IOException { - for( int numberOfAttempts=1 ; ; numberOfAttempts++ ) { - try { - tryOnceDeleteFile(f); - break; // success - } catch (IOException ex) { - boolean threadWasInterrupted = pauseBetweenDeletes(numberOfAttempts); - if( numberOfAttempts>= DELETION_MAX || threadWasInterrupted) - throw new IOException(deleteFailExceptionMessage(f, numberOfAttempts, threadWasInterrupted), ex); - } - } - } - - /** - * Deletes this file, working around most problems which might make - * this difficult. - * - * @param f - * What to delete. If a directory, it'll need to be empty. - * @throws IOException if it exists but could not be successfully deleted, - * or if it represents an invalid {@link Path}. - */ - private static void tryOnceDeleteFile(File f) throws IOException { - Path path = fileToPath(f); - try { - Files.deleteIfExists(path); - } catch (IOException e) { - // perhaps this file is read-only? - makeWritable(path); - /* - on Unix both the file and the directory that contains it has to be writable - for a file deletion to be successful. (Confirmed on Solaris 9) - - $ ls -la - total 6 - dr-xr-sr-x 2 hudson hudson 512 Apr 18 14:41 . - dr-xr-sr-x 3 hudson hudson 512 Apr 17 19:36 .. - -r--r--r-- 1 hudson hudson 469 Apr 17 19:36 manager.xml - -rw-r--r-- 1 hudson hudson 0 Apr 18 14:41 x - $ rm x - rm: x not removed: Permission denied - */ - Path parent = path.getParent(); - if (parent != null) { - makeWritable(parent); - } - try { - Files.deleteIfExists(path); - } catch (IOException e2) { - // see https://java.net/projects/hudson/lists/users/archive/2008-05/message/357 - // I suspect other processes putting files in this directory - File[] files = f.listFiles(); - if(files!=null && files.length>0) - throw new IOException("Unable to delete " + f.getPath()+" - files in dir: "+Arrays.asList(files), e2); - throw e2; - } - } - } - - /** - * Makes the file at the given path writable by any means possible. - */ - private static void makeWritable(@Nonnull Path path) throws IOException { - if (!Functions.isWindows()) { - try { - PosixFileAttributes attrs = Files.readAttributes(path, PosixFileAttributes.class); - Set newPermissions = attrs.permissions(); - newPermissions.add(PosixFilePermission.OWNER_WRITE); - Files.setPosixFilePermissions(path, newPermissions); - return; - } catch (NoSuchFileException e) { - return; - } catch (UnsupportedOperationException e) { - // PosixFileAttributes not supported, fall back to old IO. - } - } - - /** - * We intentionally do not check the return code of setWritable, because if it - * is false we prefer to rethrow the exception thrown by Files.deleteIfExists, - * which will have a more useful message than something we make up here. - */ - path.toFile().setWritable(true); + newPathRemover().forceRemoveFile(fileToPath(f)); } /** @@ -360,137 +268,7 @@ public class Util { * if the operation fails. */ public static void deleteRecursive(@Nonnull File dir) throws IOException { - for( int numberOfAttempts=1 ; ; numberOfAttempts++ ) { - try { - tryOnceDeleteRecursive(dir); - break; // success - } catch (IOException ex) { - boolean threadWasInterrupted = pauseBetweenDeletes(numberOfAttempts); - if( numberOfAttempts>= DELETION_MAX || threadWasInterrupted) - throw new IOException(deleteFailExceptionMessage(dir, numberOfAttempts, threadWasInterrupted), ex); - } - } - } - - /** - * Deletes a file or folder, throwing the first exception encountered, but - * having a go at deleting everything. i.e. it does not stop on the - * first exception, but tries (to delete) everything once. - * - * @param dir - * What to delete. If a directory, the contents will be deleted - * too. - * @throws The first exception encountered. - */ - private static void tryOnceDeleteRecursive(File dir) throws IOException { - if(!isSymlink(dir)) - tryOnceDeleteContentsRecursive(dir); - tryOnceDeleteFile(dir); - } - - /** - * Deletes a folder's contents, throwing the first exception encountered, - * but having a go at deleting everything. i.e. it does not stop - * on the first exception, but tries (to delete) everything once. - * - * @param directory - * The directory whose contents will be deleted. - * @throws The first exception encountered. - */ - private static void tryOnceDeleteContentsRecursive(File directory) throws IOException { - File[] directoryContents = directory.listFiles(); - if(directoryContents==null) - return; // the directory didn't exist in the first place - IOException firstCaught = null; - for (File child : directoryContents) { - try { - tryOnceDeleteRecursive(child); - } catch (IOException justCaught) { - if( firstCaught==null) { - firstCaught = justCaught; - } - } - } - if( firstCaught!=null ) - throw firstCaught; - } - - /** - * Pauses between delete attempts, and says if it's ok to try again. - * This does not wait if the wait time is zero or if we have tried - * too many times already. - *

- * See {@link #WAIT_BETWEEN_DELETION_RETRIES} for details of - * the pause duration.
- * See {@link #GC_AFTER_FAILED_DELETE} for when {@link System#gc()} is called. - * - * @return false if it is ok to continue trying to delete things, true if - * we were interrupted (and should stop now). - */ - @SuppressFBWarnings(value = "DM_GC", justification = "Garbage collection happens only when " - + "GC_AFTER_FAILED_DELETE is true. It's an experimental feature in Jenkins.") - private static boolean pauseBetweenDeletes(int numberOfAttemptsSoFar) { - long delayInMs; - if( numberOfAttemptsSoFar>=DELETION_MAX ) return false; - /* If the Jenkins process had the file open earlier, and it has not - * closed it then Windows won't let us delete it until the Java object - * with the open stream is Garbage Collected, which can result in builds - * failing due to "file in use" on Windows despite working perfectly - * well on other OSs. */ - if (GC_AFTER_FAILED_DELETE) { - System.gc(); - } - if (WAIT_BETWEEN_DELETION_RETRIES>=0) { - delayInMs = WAIT_BETWEEN_DELETION_RETRIES; - } else { - delayInMs = -numberOfAttemptsSoFar*WAIT_BETWEEN_DELETION_RETRIES; - } - if (delayInMs<=0) - return Thread.interrupted(); - try { - Thread.sleep(delayInMs); - return false; - } catch (InterruptedException e) { - return true; - } - } - - /** - * Creates a "couldn't delete file" message that explains how hard we tried. - * See {@link #DELETION_MAX}, {@link #WAIT_BETWEEN_DELETION_RETRIES} - * and {@link #GC_AFTER_FAILED_DELETE} for more details. - */ - private static String deleteFailExceptionMessage(File whatWeWereTryingToRemove, int retryCount, boolean wasInterrupted) { - StringBuilder sb = new StringBuilder(); - sb.append("Unable to delete '"); - sb.append(whatWeWereTryingToRemove); - sb.append("'. Tried "); - sb.append(retryCount); - sb.append(" time"); - if( retryCount!=1 ) sb.append('s'); - if( DELETION_MAX>1 ) { - sb.append(" (of a maximum of "); - sb.append(DELETION_MAX); - sb.append(')'); - if( GC_AFTER_FAILED_DELETE ) - sb.append(" garbage-collecting"); - if( WAIT_BETWEEN_DELETION_RETRIES!=0 && GC_AFTER_FAILED_DELETE ) - sb.append(" and"); - if( WAIT_BETWEEN_DELETION_RETRIES!=0 ) { - sb.append(" waiting "); - sb.append(getTimeSpanString(Math.abs(WAIT_BETWEEN_DELETION_RETRIES))); - if( WAIT_BETWEEN_DELETION_RETRIES<0 ) { - sb.append("-"); - sb.append(getTimeSpanString(Math.abs(WAIT_BETWEEN_DELETION_RETRIES)*DELETION_MAX)); - } - } - if( WAIT_BETWEEN_DELETION_RETRIES!=0 || GC_AFTER_FAILED_DELETE) - sb.append(" between attempts"); - } - if( wasInterrupted ) - sb.append(". The delete operation was interrupted before it completed successfully"); - sb.append('.'); - return sb.toString(); + newPathRemover().forceRemoveRecursive(fileToPath(dir)); } /* @@ -1797,6 +1575,10 @@ public class Util { @Restricted(value = NoExternalUse.class) static boolean GC_AFTER_FAILED_DELETE = SystemProperties.getBoolean(Util.class.getName() + ".performGCOnFailedDelete"); + private static PathRemover newPathRemover() { + return PathRemover.newRobustRemover(DELETION_MAX - 1, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); + } + /** * If this flag is true, native implementations of {@link FilePath#chmod} * and {@link hudson.util.IOUtils#mode} are used instead of NIO. diff --git a/core/src/main/java/jenkins/util/io/PathRemover.java b/core/src/main/java/jenkins/util/io/PathRemover.java index dd237d6745..a4f7075ce8 100644 --- a/core/src/main/java/jenkins/util/io/PathRemover.java +++ b/core/src/main/java/jenkins/util/io/PathRemover.java @@ -24,7 +24,9 @@ package jenkins.util.io; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.Functions; +import hudson.Util; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -45,13 +47,21 @@ import java.util.Set; @Restricted(NoExternalUse.class) public class PathRemover { - private final RetryStrategy retryStrategy; - - public PathRemover() { - this(ignored -> false); + public static PathRemover newSimpleRemover() { + return new PathRemover(ignored -> false); } - public PathRemover(@Nonnull RetryStrategy retryStrategy) { + public static PathRemover newRemoverWithStrategy(@Nonnull RetryStrategy retryStrategy) { + return new PathRemover(retryStrategy); + } + + public static PathRemover newRobustRemover(int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { + return new PathRemover(new PausingGCRetryStrategy(maxRetries < 1 ? 1 : maxRetries, gcAfterFailedRemove, waitBetweenRetries)); + } + + private final RetryStrategy retryStrategy; + + private PathRemover(@Nonnull RetryStrategy retryStrategy) { this.retryStrategy = retryStrategy; } @@ -100,6 +110,80 @@ public class PathRemover { return sb.toString(); } } + + private static class PausingGCRetryStrategy implements RetryStrategy { + private final int maxRetries; + private final boolean gcAfterFailedRemove; + private final long waitBetweenRetries; + private final ThreadLocal interrupted = ThreadLocal.withInitial(() -> false); + + private PausingGCRetryStrategy(int maxRetries, boolean gcAfterFailedRemove, long waitBetweenRetries) { + this.maxRetries = maxRetries; + this.gcAfterFailedRemove = gcAfterFailedRemove; + this.waitBetweenRetries = waitBetweenRetries; + } + + @SuppressFBWarnings(value = "DM_GC", justification = "Garbage collection happens only when " + + "GC_AFTER_FAILED_DELETE is true. It's an experimental feature in Jenkins.") + private void gcIfEnabled() { + /* If the Jenkins process had the file open earlier, and it has not + * closed it then Windows won't let us delete it until the Java object + * with the open stream is Garbage Collected, which can result in builds + * failing due to "file in use" on Windows despite working perfectly + * well on other OSs. */ + if (gcAfterFailedRemove) System.gc(); + } + + @Override + public boolean shouldRetry(int retriesAttempted) { + if (retriesAttempted >= maxRetries) return false; + gcIfEnabled(); + long delayMillis = waitBetweenRetries >= 0 ? waitBetweenRetries : -retriesAttempted * waitBetweenRetries; + if (delayMillis <= 0) return !Thread.interrupted(); + try { + Thread.sleep(delayMillis); + return true; + } catch (InterruptedException e) { + interrupted.set(true); + return false; + } + } + + @Override + public String failureMessage(@Nonnull Path fileToRemove, int retryCount) { + StringBuilder sb = new StringBuilder(); + sb.append("Unable to delete '"); + sb.append(fileToRemove); + sb.append("'. Tried "); + sb.append(retryCount); + sb.append(" time"); + if (retryCount != 1) sb.append('s'); + if (maxRetries > 0) { + sb.append(" (of a maximum of "); + sb.append(maxRetries + 1); + sb.append(')'); + if (gcAfterFailedRemove) + sb.append(" garbage-collecting"); + if (waitBetweenRetries != 0 && gcAfterFailedRemove) + sb.append(" and"); + if (waitBetweenRetries != 0) { + sb.append(" waiting "); + sb.append(Util.getTimeSpanString(Math.abs(waitBetweenRetries))); + if (waitBetweenRetries < 0) { + sb.append("-"); + sb.append(Util.getTimeSpanString(Math.abs(waitBetweenRetries) * (maxRetries + 1))); + } + } + if (waitBetweenRetries != 0 || gcAfterFailedRemove) + sb.append(" between attempts"); + } + if (interrupted.get()) + sb.append(". The delete operation was interrupted before it completed successfully"); + sb.append('.'); + interrupted.set(false); + return sb.toString(); + } + } private static Optional tryRemoveFile(@Nonnull Path path) { try { diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index 051e9b7472..36586408e5 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -27,37 +27,23 @@ package hudson; import java.text.ParseException; import java.text.SimpleDateFormat; import java.util.Date; -import java.util.List; import java.util.Map; import java.util.HashMap; import java.util.Locale; import java.util.Properties; -import java.util.concurrent.Callable; -import java.util.concurrent.atomic.AtomicReference; import java.io.ByteArrayOutputStream; import java.io.File; -import java.io.FileInputStream; import java.io.IOException; -import java.io.InputStream; -import java.nio.file.FileSystemException; -import java.nio.file.Files; -import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermissions; -import static org.hamcrest.CoreMatchers.allOf; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.hasItem; -import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.startsWith; import static org.junit.Assert.*; -import org.apache.commons.io.FileUtils; import org.hamcrest.BaseMatcher; import org.hamcrest.Description; import org.junit.Assume; -import org.junit.Ignore; import org.junit.Test; import org.junit.rules.ExpectedException; import org.jvnet.hudson.test.Issue; @@ -67,8 +53,6 @@ import hudson.util.StreamTaskListener; import org.junit.Rule; import org.junit.rules.TemporaryFolder; -import com.google.common.collect.Lists; - /** * @author Kohsuke Kawaguchi */ @@ -284,284 +268,6 @@ public class UtilTest { assertTrue(Util.isSymlink(new File(d, "junction"))); } - @Test - public void testDeleteFile() throws Exception { - File f = tmp.newFile(); - // Test: File is deleted - mkfiles(f); - Util.deleteFile(f); - assertFalse("f exists after calling Util.deleteFile", f.exists()); - } - - @Test - public void testDeleteFile_onWindows() throws Exception { - Assume.assumeTrue(Functions.isWindows()); - final int defaultDeletionMax = Util.DELETION_MAX; - try { - File f = tmp.newFile(); - // Test: If we cannot delete a file, we throw explaining why - mkfiles(f); - lockFileForDeletion(f); - Util.DELETION_MAX = 1; - try { - Util.deleteFile(f); - fail("should not have been deletable"); - } catch (IOException x) { - assertThat(calcExceptionHierarchy(x), hasItem(FileSystemException.class)); - assertThat(x.getMessage(), containsString(f.getPath())); - } - } finally { - Util.DELETION_MAX = defaultDeletionMax; - unlockFilesForDeletion(); - } - } - - @Test - public void testDeleteFileReadOnly() throws Exception { - // Removing the calls to Util#makeWritable in Util#tryOnceDeleteFile should cause this test to fail. - Path file = tmp.newFolder().toPath().resolve("file.tmp"); - Files.createDirectories(file.getParent()); - Files.createFile(file); - // Using old IO so the test can run on Windows. - file.getParent().toFile().setWritable(false); - file.toFile().setWritable(false); - Util.deleteFile(file.toFile()); - assertFalse(Files.exists(file)); - } - - @Test - public void testDeleteFileDoesNotExist() throws Exception { - Path file = tmp.newFolder().toPath().resolve("file.tmp"); - assertFalse(Files.exists(file)); - // Should not throw an exception. - Util.deleteFile(file.toFile()); - } - - @Test - public void testDeleteContentsRecursive() throws Exception { - final File dir = tmp.newFolder(); - final File d1 = new File(dir, "d1"); - final File d2 = new File(dir, "d2"); - final File f1 = new File(dir, "f1"); - final File d1f1 = new File(d1, "d1f1"); - final File d2f2 = new File(d2, "d1f2"); - // Test: Files and directories are deleted - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - Util.deleteContentsRecursive(dir); - assertTrue("dir exists", dir.exists()); - assertFalse("d1 exists", d1.exists()); - assertFalse("d2 exists", d2.exists()); - assertFalse("f1 exists", f1.exists()); - } - - @Test - public void testDeleteContentsRecursive_onWindows() throws Exception { - Assume.assumeTrue(Functions.isWindows()); - final File dir = tmp.newFolder(); - final File d1 = new File(dir, "d1"); - final File d2 = new File(dir, "d2"); - final File f1 = new File(dir, "f1"); - final File d1f1 = new File(d1, "d1f1"); - final File d2f2 = new File(d2, "d1f2"); - final int defaultDeletionMax = Util.DELETION_MAX; - final int defaultDeletionWait = Util.WAIT_BETWEEN_DELETION_RETRIES; - final boolean defaultDeletionGC = Util.GC_AFTER_FAILED_DELETE; - try { - // Test: If we cannot delete a file, we throw - // but still deletes everything it can - // even if we are not retrying deletes. - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - lockFileForDeletion(d1f1); - Util.GC_AFTER_FAILED_DELETE = false; - Util.DELETION_MAX = 2; - Util.WAIT_BETWEEN_DELETION_RETRIES = 0; - expectedException.expectMessage(allOf( - containsString(dir.getPath()), - not(containsString("interrupted")), - containsString("Tried 2 times (of a maximum of 2)."), - not(containsString("garbage-collecting")), - not(containsString("wait")) - )); - Util.deleteContentsRecursive(dir); - assertFalse("d2 should not exist", d2.exists()); - assertFalse("f1 should not exist", f1.exists()); - assertFalse("d1f2 should not exist", d2f2.exists()); - } finally { - Util.DELETION_MAX = defaultDeletionMax; - Util.WAIT_BETWEEN_DELETION_RETRIES = defaultDeletionWait; - Util.GC_AFTER_FAILED_DELETE = defaultDeletionGC; - unlockFilesForDeletion(); - } - } - - @Test - public void testDeleteRecursive() throws Exception { - final File dir = tmp.newFolder(); - final File d1 = new File(dir, "d1"); - final File d2 = new File(dir, "d2"); - final File f1 = new File(dir, "f1"); - final File d1f1 = new File(d1, "d1f1"); - final File d2f2 = new File(d2, "d1f2"); - // Test: Files and directories are deleted - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - Util.deleteRecursive(dir); - assertFalse("dir exists", dir.exists()); - } - - @Ignore("JENKINS-55016") - @Test - public void testDeleteRecursive_onWindows() throws Exception { - Assume.assumeTrue(Functions.isWindows()); - final File dir = tmp.newFolder(); - final File d1 = new File(dir, "d1"); - final File d2 = new File(dir, "d2"); - final File f1 = new File(dir, "f1"); - final File d1f1 = new File(d1, "d1f1"); - final File d2f2 = new File(d2, "d1f2"); - final int defaultDeletionMax = Util.DELETION_MAX; - final int defaultDeletionWait = Util.WAIT_BETWEEN_DELETION_RETRIES; - final boolean defaultDeletionGC = Util.GC_AFTER_FAILED_DELETE; - try { - // Test: If we cannot delete a file, we throw - // but still deletes everything it can - // even if we are not retrying deletes. - // (And when we are not retrying deletes, - // we do not do the "between retries" delay) - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - lockFileForDeletion(d1f1); - Util.GC_AFTER_FAILED_DELETE = false; - Util.DELETION_MAX = 1; - Util.WAIT_BETWEEN_DELETION_RETRIES = 10000; // long enough to notice - long timeWhenDeletionStarted = System.currentTimeMillis(); - expectedException.expectMessage(allOf( - containsString(dir.getPath()), - not(containsString("interrupted")), - not(containsString("maximum of")), - not(containsString("garbage-collecting")) - )); - Util.deleteRecursive(dir); - long timeWhenDeletionEnded = System.currentTimeMillis(); - assertTrue("dir exists", dir.exists()); - assertTrue("d1 exists", d1.exists()); - assertTrue("d1f1 exists", d1f1.exists()); - assertFalse("d2 should not exist", d2.exists()); - assertFalse("f1 should not exist", f1.exists()); - assertFalse("d1f2 should not exist", d2f2.exists()); - long actualTimeSpentDeleting = timeWhenDeletionEnded - timeWhenDeletionStarted; - assertTrue("did not wait - took " + actualTimeSpentDeleting + "ms", actualTimeSpentDeleting<1000L); - unlockFileForDeletion(d1f1); - // Deletes get retried if they fail 1st time around, - // allowing the operation to succeed on subsequent attempts. - // Note: This is what bug JENKINS-15331 is all about. - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - lockFileForDeletion(d2f2); - Util.DELETION_MAX=4; - Util.WAIT_BETWEEN_DELETION_RETRIES = 100; - Thread unlockAfterDelay = new Thread("unlockFileAfterDelay") { - public void run() { - try { - Thread.sleep(Util.WAIT_BETWEEN_DELETION_RETRIES); - unlockFileForDeletion(d2f2); - } catch( Exception x ) { /* ignored */ } - } - }; - unlockAfterDelay.start(); - Util.deleteRecursive(dir); - assertFalse("dir should have been deleted", dir.exists()); - unlockAfterDelay.join(); - // An interrupt aborts the delete and makes it fail, even - // if we had been told to retry a lot. - mkdirs(dir, d1, d2); - mkfiles(f1, d1f1, d2f2); - lockFileForDeletion(d1f1); - Util.DELETION_MAX=10; - Util.WAIT_BETWEEN_DELETION_RETRIES = -1000; - Util.GC_AFTER_FAILED_DELETE = true; - final AtomicReference thrown = new AtomicReference(); - Thread deleteToBeInterrupted = new Thread("deleteToBeInterrupted") { - public void run() { - try { Util.deleteRecursive(dir); } - catch( Throwable x ) { thrown.set(x); } - } - }; - deleteToBeInterrupted.start(); - deleteToBeInterrupted.interrupt(); - deleteToBeInterrupted.join(500); - assertFalse("deletion stopped", deleteToBeInterrupted.isAlive()); - assertTrue("d1f1 still exists", d1f1.exists()); - unlockFileForDeletion(d1f1); - Throwable deletionInterruptedEx = thrown.get(); - assertThat(deletionInterruptedEx, instanceOf(IOException.class)); - assertThat(deletionInterruptedEx.getMessage(), allOf(containsString("interrupted"), containsString("maximum of " + Util.DELETION_MAX), containsString("garbage-collecting"))); - } finally { - Util.DELETION_MAX = defaultDeletionMax; - Util.WAIT_BETWEEN_DELETION_RETRIES = defaultDeletionWait; - Util.GC_AFTER_FAILED_DELETE = defaultDeletionGC; - unlockFilesForDeletion(); - } - } - - /** Creates multiple directories. */ - private static void mkdirs(File... dirs) { - for( File d : dirs ) { - d.mkdir(); - assertTrue(d.getPath(), d.isDirectory()); - } - } - - /** Creates multiple files, each containing their filename as text content. */ - private static void mkfiles(File... files) throws IOException { - for( File f : files ) - FileUtils.write(f, f.getName()); - } - - /** Means of unlocking all the files we have locked, indexed by {@link File}. */ - private final Map> unlockFileCallables = new HashMap>(); - - /** Prevents a file from being deleted, so we can stress the deletion code's retries. */ - private void lockFileForDeletion(File f) throws IOException, InterruptedException { - assert !unlockFileCallables.containsKey(f) : f + " is already locked." ; - // Limitation: Only works on Windows. On unix we can delete anything we can create. - // On unix, can't use "chmod a-w" on the dir as the code-under-test undoes that. - // On unix, can't use "chattr +i" because that needs root. - // On unix, can't use "chattr +u" because ext fs ignores it. - // On Windows, can't use FileChannel.lock() because that doesn't block deletion - // On Windows, we can't delete files that are open for reading, so we use that. - // NOTE: This is a hack in any case as there is no guarantee that all Windows filesystems - // will enforce blocking deletion on open files... just that the ones we normally - // test with seem to block. - assert Functions.isWindows(); - final InputStream s = new FileInputStream(f); // intentional use of FileInputStream - unlockFileCallables.put(f, new Callable() { - public Void call() throws IOException { s.close(); return null; }; - }); - } - - /** Undoes a call to {@link #lockFileForDeletion(File)}. */ - private void unlockFileForDeletion(File f) throws Exception { - unlockFileCallables.remove(f).call(); - } - - /** Undoes all calls to {@link #lockFileForDeletion(File)}. */ - private void unlockFilesForDeletion() throws Exception { - while( !unlockFileCallables.isEmpty() ) { - unlockFileForDeletion(unlockFileCallables.keySet().iterator().next()); - } - } - - /** Returns all classes in the exception hierarchy. */ - private static Iterable> calcExceptionHierarchy(Throwable t) { - final List> result = Lists.newArrayList(); - for( ; t!=null ; t = t.getCause()) - result.add(t.getClass()); - return result; - } - @Test public void testHtmlEscape() { assertEquals("
", Util.escape("\n")); diff --git a/core/src/test/java/jenkins/util/io/PathRemoverTest.java b/core/src/test/java/jenkins/util/io/PathRemoverTest.java index 492633b907..614475a25f 100644 --- a/core/src/test/java/jenkins/util/io/PathRemoverTest.java +++ b/core/src/test/java/jenkins/util/io/PathRemoverTest.java @@ -52,7 +52,7 @@ public class PathRemoverTest { File file = tmp.newFile(); touchWithFileName(file); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); remover.forceRemoveFile(file.toPath()); assertFalse(file.exists()); @@ -66,7 +66,7 @@ public class PathRemoverTest { touchWithFileName(file); locker.acquireLock(file); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); expectedException.expectMessage(containsString(file.getPath())); remover.forceRemoveFile(file.toPath()); @@ -81,7 +81,7 @@ public class PathRemoverTest { assertTrue(file.setWritable(false)); assertTrue(dir.setWritable(false)); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); remover.forceRemoveFile(file.toPath()); assertFalse(file.exists()); @@ -93,7 +93,7 @@ public class PathRemoverTest { File file = new File(dir, "invalid.file"); assertFalse(file.exists()); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); remover.forceRemoveFile(file.toPath()); assertFalse(file.exists()); @@ -110,7 +110,7 @@ public class PathRemoverTest { mkdirs(d1, d2); touchWithFileName(f1, d1f1, d2f2); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); remover.forceRemoveDirectoryContents(dir.toPath()); assertTrue(dir.exists()); @@ -132,7 +132,7 @@ public class PathRemoverTest { touchWithFileName(f1, d1f1, d2f2); try (FileLocker locker = new FileLocker()) { locker.acquireLock(d1f1); - PathRemover remover = new PathRemover(retriesAttempted -> retriesAttempted < 1); + PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> retriesAttempted < 1); expectedException.expectMessage(allOf( containsString(dir.getPath()), containsString("Tried 1 time.") @@ -155,7 +155,7 @@ public class PathRemoverTest { mkdirs(d1, d2); touchWithFileName(f1, d1f1, d2f2); - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); remover.forceRemoveRecursive(dir.toPath()); assertFalse(dir.exists()); @@ -178,7 +178,7 @@ public class PathRemoverTest { // but still deletes everything it can // even if we are not retrying deletes. try (AutoCloseable ignored = locker.acquireLock(d1f1)) { - PathRemover remover = new PathRemover(); + PathRemover remover = PathRemover.newSimpleRemover(); expectedException.expectMessage(containsString(dir.getPath())); remover.forceRemoveRecursive(dir.toPath()); assertTrue(dir.exists()); @@ -207,7 +207,7 @@ public class PathRemoverTest { } catch (Exception ignored) { } }).start(); - PathRemover remover = new PathRemover(retriesAttempted -> { + PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> { if (retriesAttempted == 0) { lockedFileExists.set(d2f2.exists()); readyToUnlockSignal.notifyAll(); @@ -233,7 +233,7 @@ public class PathRemoverTest { locker.acquireLock(d1f1); AtomicReference interrupted = new AtomicReference<>(); AtomicReference removed = new AtomicReference<>(); - PathRemover remover = new PathRemover(retriesAttempted -> { + PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> { try { TimeUnit.SECONDS.sleep(retriesAttempted + 1); return true;