diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 8403a8fd2e..2094d04020 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -43,6 +43,9 @@ import org.apache.tools.ant.taskdefs.Chmod; import org.apache.tools.ant.taskdefs.Copy; import org.apache.tools.ant.types.FileSet; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + import jnr.posix.FileStat; import jnr.posix.POSIX; @@ -214,24 +217,54 @@ public class Util { /** * Deletes the contents of the given directory (but not the directory itself) * recursively. + * It does not take no for an answer - if necessary, it will have multiple + * attempts at deleting things. * * @throws IOException * if the operation fails. */ public static void deleteContentsRecursive(@Nonnull File file) throws IOException { - File[] files = file.listFiles(); - if(files==null) - return; // the directory didn't exist in the first place - for (File child : files) - deleteRecursive(child); + 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); + } + } } /** * Deletes this file (and does not take no for an answer). + * If necessary, it will have multiple attempts at deleting things. + * * @param f a file to delete * @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 + */ + private static void tryOnceDeleteFile(File f) throws IOException { if (!f.delete()) { if(!f.exists()) // we are trying to delete a file that no longer exists, so this is not an error @@ -300,21 +333,146 @@ public class Util { } + /** + * Deletes the given directory (including its contents) recursively. + * It does not take no for an answer - if necessary, it will have multiple + * attempts at deleting things. + * + * @throws IOException + * if the operation fails. + */ public static void deleteRecursive(@Nonnull File dir) throws IOException { - if(!isSymlink(dir)) - deleteContentsRecursive(dir); - try { - deleteFile(dir); - } catch (IOException e) { - // if some of the child directories are big, it might take long enough to delete that - // it allows others to create new files, causing problemsl ike JENKINS-10113 - // so give it one more attempt before we give up. - if(!isSymlink(dir)) - deleteContentsRecursive(dir); - deleteFile(dir); + 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). + */ + 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(); + } + /* * Copyright 2001-2004 The Apache Software Foundation. * @@ -1465,4 +1623,57 @@ public class Util { public static boolean NO_SYMLINK = Boolean.getBoolean(Util.class.getName()+".noSymLink"); public static boolean SYMLINK_ESCAPEHATCH = Boolean.getBoolean(Util.class.getName()+".symlinkEscapeHatch"); + + /** + * The number of times we will attempt to delete files/directory trees + * before giving up and throwing an exception.
+ * Specifying a value less than 1 is invalid and will be treated as if + * a value of 1 (i.e. one attempt, no retries) was specified. + *

+ * e.g. if some of the child directories are big, it might take long enough + * to delete that it allows others to create new files in the directory we + * are trying to empty, causing problems like JENKINS-10113. + * Or, if we're on Windows, then deletes can fail for transient reasons + * regardless of external activity; see JENKINS-15331. + * Whatever the reason, this allows us to do multiple attempts before we + * give up, thus improving build reliability. + */ + @Restricted(value = NoExternalUse.class) + static int DELETION_MAX = Math.max(1, Integer.getInteger(Util.class.getName() + ".maxFileDeletionRetries", 3).intValue()); + + /** + * The time (in milliseconds) that we will wait between attempts to + * delete files when retrying.
+ * This has no effect unless {@link #DELETION_MAX} is non-zero. + *

+ * If zero, we will not delay between attempts.
+ * If negative, we will wait an (linearly) increasing multiple of this value + * between attempts. + */ + @Restricted(value = NoExternalUse.class) + static int WAIT_BETWEEN_DELETION_RETRIES = Integer.getInteger(Util.class.getName() + ".deletionRetryWait", 100).intValue(); + + /** + * If this flag is set to true then we will request a garbage collection + * after a deletion failure before we next retry the delete.
+ * It defaults to false and is ignored unless + * {@link #DELETION_MAX} is greater than 1. + *

+ * Setting this flag to true may resolve some problems on Windows, + * and also for directory trees residing on an NFS share, but it can + * have a negative impact on performance and may have no effect at all (GC + * behavior is JVM-specific). + *

+ * Warning: This should only ever be used if you find that your builds are + * failing because Jenkins is unable to delete files, that this failure is + * because Jenkins itself has those files locked "open", and even then it + * should only be used on slaves with relatively few executors (because the + * garbage collection can impact the performance of all job executors on + * that slave).
+ * i.e. Setting this flag is a act of last resort - it is not + * recommended, and should not be used on the main Jenkins server + * unless you can tolerate the performance impact. + */ + @Restricted(value = NoExternalUse.class) + static boolean GC_AFTER_FAILED_DELETE = Boolean.getBoolean(Util.class.getName() + ".performGCOnFailedDelete"); } diff --git a/core/src/test/java/hudson/UtilTest.java b/core/src/test/java/hudson/UtilTest.java index f413ffe9d9..1ba20213eb 100644 --- a/core/src/test/java/hudson/UtilTest.java +++ b/core/src/test/java/hudson/UtilTest.java @@ -30,14 +30,17 @@ 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 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.not; import static org.junit.Assert.*; @@ -274,11 +277,13 @@ public class UtilTest { } catch (ClassNotFoundException x) { throw new AssumptionViolatedException("prior to JDK 7", x); } + 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"); @@ -287,6 +292,65 @@ public class UtilTest { assertThat(x.getMessage(), containsString(f.getPath())); } } finally { + Util.DELETION_MAX = defaultDeletionMax; + unlockFilesForDeletion(); + } + } + + @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; + try { + Util.deleteContentsRecursive(dir); + fail("Expected IOException"); + } catch (IOException x) { + assertFalse("d2 should not exist", d2.exists()); + assertFalse("f1 should not exist", f1.exists()); + assertFalse("d1f2 should not exist", d2f2.exists()); + assertThat(x.getMessage(), containsString(dir.getPath())); + assertThat(x.getMessage(), allOf(not(containsString("interrupted")), containsString("Tried 2 times (of a maximum of 2)."), not(containsString("garbage-collecting")), not(containsString("wait")))); + } + } finally { + Util.DELETION_MAX = defaultDeletionMax; + Util.WAIT_BETWEEN_DELETION_RETRIES = defaultDeletionWait; + Util.GC_AFTER_FAILED_DELETE = defaultDeletionGC; unlockFilesForDeletion(); } } @@ -315,21 +379,87 @@ public class UtilTest { 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(); try { Util.deleteRecursive(dir); fail("Expected IOException"); } catch (IOException x) { + 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()); assertThat(x.getMessage(), containsString(dir.getPath())); + assertThat(x.getMessage(), allOf(not(containsString("interrupted")), not(containsString("maximum of")), not(containsString("garbage-collecting")))); + 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 deleteToBeInterupted = new Thread("deleteToBeInterupted") { + public void run() { + try { Util.deleteRecursive(dir); } + catch( Throwable x ) { thrown.set(x); } + } + }; + deleteToBeInterupted.start(); + deleteToBeInterupted.interrupt(); + deleteToBeInterupted.join(500); + assertFalse("deletion stopped", deleteToBeInterupted.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(); } }