Refactor FileLocker into JUnit rule

Also fixes a bug where locked files are not cleaned up in the cleanup.
Signed-off-by: NMatt Sicker <boards@gmail.com>

core/src/main/java/jenkins/util/io/CompositeIOException.java

@@ -24,13 +24,18 @@
 
 package jenkins.util.io;
 
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
 import javax.annotation.Nonnull;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.io.PrintWriter;
+import java.io.UncheckedIOException;
 import java.util.Arrays;
 import java.util.List;
 
+@Restricted(NoExternalUse.class)
 public class CompositeIOException extends IOException {
     private final List<IOException> exceptions;
 
@@ -62,4 +67,8 @@ public class CompositeIOException extends IOException {
             exception.printStackTrace(s);
         }
     }
+
+    public UncheckedIOException asUncheckedIOException() {
+        return new UncheckedIOException(this);
+    }
 }

core/src/test/java/jenkins/util/io/FileLocker.java → core/src/test/java/jenkins/util/io/FileLockerRule.java

@@ -25,11 +25,17 @@
 package jenkins.util.io;
 
 import hudson.Functions;
+import org.junit.rules.ExternalResource;
 
+import javax.annotation.Nonnull;
+import java.io.Closeable;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.hasKey;
@@ -41,25 +47,34 @@ import static org.junit.Assert.assertTrue;
  * Helper class for tracking which files are locked.
  * Only useful in Windows environments as POSIX seems to allow locked files to be deleted.
  */
-public class FileLocker implements AutoCloseable {
-    private final Map<File, AutoCloseable> locks = new HashMap<>();
+public class FileLockerRule extends ExternalResource {
+    private final Map<File, Closeable> locks = new HashMap<>();
 
-    public synchronized void acquireLock(File file) throws IOException {
+    @Override
+    protected void after() {
+        List<IOException> exceptions = new ArrayList<>();
+        Iterator<Closeable> it = locks.values().iterator();
+        while (it.hasNext()) {
+            try (Closeable ignored = it.next()) {
+                it.remove();
+            } catch (IOException e) {
+                exceptions.add(e);
+            }
+        }
+        if (!exceptions.isEmpty())
+            throw new CompositeIOException("Could not unlock all files", exceptions).asUncheckedIOException();
+    }
+
+    public synchronized void acquireLock(@Nonnull File file) throws IOException {
         assertTrue(Functions.isWindows());
         assertThat(file + " is already locked.", locks, not(hasKey(file)));
-        AutoCloseable lock = new FileInputStream(file);
+        Closeable lock = new FileInputStream(file);
         locks.put(file, lock);
     }
 
-    public synchronized void releaseLock(File file) throws Exception {
+    public synchronized void releaseLock(@Nonnull File file) throws Exception {
+        assertTrue(Functions.isWindows());
         assertThat(file + " is not locked.", locks, hasKey(file));
-        locks.get(file).close();
-    }
-
-    @Override
-    public synchronized void close() throws Exception {
-        while (!locks.isEmpty()) {
-            releaseLock(locks.keySet().iterator().next());
-        }
+        locks.remove(file).close();
     }
 }

core/src/test/java/jenkins/util/io/PathRemoverTest.java

@@ -48,6 +48,7 @@ public class PathRemoverTest {
     @Rule public TemporaryFolder tmp = new TemporaryFolder();
     @Rule public ExpectedException expectedException = ExpectedException.none();
     @Rule public Timeout timeout = new Timeout(10, TimeUnit.SECONDS);
+    @Rule public FileLockerRule locker = new FileLockerRule();
 
     @Test
     public void testForceRemoveFile() throws IOException {
@@ -63,16 +64,14 @@ public class PathRemoverTest {
     @Test
     public void testForceRemoveFile_LockedFile() throws Exception {
         assumeTrue(Functions.isWindows());
-        try (FileLocker locker = new FileLocker()) {
-            File file = tmp.newFile();
-            touchWithFileName(file);
-            locker.acquireLock(file);
+        File file = tmp.newFile();
+        touchWithFileName(file);
+        locker.acquireLock(file);
 
-            PathRemover remover = PathRemover.newSimpleRemover();
-            expectedException.expectMessage(containsString(file.getPath()));
+        PathRemover remover = PathRemover.newSimpleRemover();
+        expectedException.expectMessage(containsString(file.getPath()));
 
-            remover.forceRemoveFile(file.toPath());
-        }
+        remover.forceRemoveFile(file.toPath());
     }
 
     @Test
@@ -132,18 +131,16 @@ public class PathRemoverTest {
         File d2f2 = new File(d2, "d1f2");
         mkdirs(d1, d2);
         touchWithFileName(f1, d1f1, d2f2);
-        try (FileLocker locker = new FileLocker()) {
-            locker.acquireLock(d1f1);
-            PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> retriesAttempted < 1);
-            expectedException.expectMessage(allOf(
-                    containsString(dir.getPath()),
-                    containsString("Tried 1 time.")
-            ));
-            remover.forceRemoveDirectoryContents(dir.toPath());
-            assertFalse(d2.exists());
-            assertFalse(f1.exists());
-            assertFalse(d2f2.exists());
-        }
+        locker.acquireLock(d1f1);
+        PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> retriesAttempted < 1);
+        expectedException.expectMessage(allOf(
+                containsString(dir.getPath()),
+                containsString("Tried 1 time.")
+        ));
+        remover.forceRemoveDirectoryContents(dir.toPath());
+        assertFalse(d2.exists());
+        assertFalse(f1.exists());
+        assertFalse(d2f2.exists());
     }
 
     @Test
@@ -175,18 +172,16 @@ public class PathRemoverTest {
         mkdirs(d1, d2);
         touchWithFileName(f1, d1f1, d2f2);
 
-        try (FileLocker locker = new FileLocker()) {
-            locker.acquireLock(d1f1);
-            PathRemover remover = PathRemover.newSimpleRemover();
-            expectedException.expectMessage(containsString(dir.getPath()));
-            remover.forceRemoveRecursive(dir.toPath());
-            assertTrue(dir.exists());
-            assertTrue(d1.exists());
-            assertTrue(d1f1.exists());
-            assertFalse(d2.exists());
-            assertFalse(d2f2.exists());
-            assertFalse(f1.exists());
-        }
+        locker.acquireLock(d1f1);
+        PathRemover remover = PathRemover.newSimpleRemover();
+        expectedException.expectMessage(containsString(dir.getPath()));
+        remover.forceRemoveRecursive(dir.toPath());
+        assertTrue(dir.exists());
+        assertTrue(d1.exists());
+        assertTrue(d1f1.exists());
+        assertFalse(d2.exists());
+        assertFalse(d2f2.exists());
+        assertFalse(f1.exists());
     }
 
     @Test
@@ -200,38 +195,36 @@ public class PathRemoverTest {
         File d2f2 = new File(d2, "d1f2");
         mkdirs(d1, d2);
         touchWithFileName(f1, d1f1, d2f2);
-        try (FileLocker locker = new FileLocker()) {
-            locker.acquireLock(d2f2);
-            Object readyToUnlockSignal = new Object();
-            Object readyToDeleteSignal = new Object();
-            AtomicBoolean lockedFileExists = new AtomicBoolean();
-            Thread thread = new Thread(() -> {
+        locker.acquireLock(d2f2);
+        Object readyToUnlockSignal = new Object();
+        Object readyToDeleteSignal = new Object();
+        AtomicBoolean lockedFileExists = new AtomicBoolean();
+        Thread thread = new Thread(() -> {
+            try {
+                readyToUnlockSignal.wait();
+                locker.releaseLock(d2f2);
+                readyToDeleteSignal.notifyAll();
+            } catch (Exception ignored) {
+            }
+        });
+        thread.start();
+        PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> {
+            if (retriesAttempted == 0) {
+                lockedFileExists.set(d2f2.exists());
+                readyToUnlockSignal.notifyAll();
                 try {
-                    readyToUnlockSignal.wait();
-                    locker.releaseLock(d2f2);
-                    readyToDeleteSignal.notifyAll();
-                } catch (Exception ignored) {
-                }
-            });
-            thread.start();
-            PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> {
-                if (retriesAttempted == 0) {
-                    lockedFileExists.set(d2f2.exists());
-                    readyToUnlockSignal.notifyAll();
-                    try {
-                        readyToDeleteSignal.wait();
-                        return true;
-                    } catch (InterruptedException e) {
-                        return false;
-                    }
+                    readyToDeleteSignal.wait();
+                    return true;
+                } catch (InterruptedException e) {
+                    return false;
                 }
-                return false;
-            });
-            remover.forceRemoveRecursive(dir.toPath());
-            thread.join();
-            assertTrue(lockedFileExists.get());
-            assertFalse(dir.exists());
-        }
+            }
+            return false;
+        });
+        remover.forceRemoveRecursive(dir.toPath());
+        thread.join();
+        assertTrue(lockedFileExists.get());
+        assertFalse(dir.exists());
     }
 
     @Test
@@ -245,37 +238,35 @@ public class PathRemoverTest {
         File d2f2 = new File(d2, "d1f2");
         mkdirs(d1, d2);
         touchWithFileName(f1, d1f1, d2f2);
-        try (FileLocker locker = new FileLocker()) {
-            locker.acquireLock(d1f1);
-            AtomicReference<InterruptedException> interrupted = new AtomicReference<>();
-            AtomicReference<IOException> removed = new AtomicReference<>();
-            PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> {
-                try {
-                    TimeUnit.SECONDS.sleep(retriesAttempted + 1);
-                    return true;
-                } catch (InterruptedException e) {
-                    interrupted.set(e);
-                    return false;
-                }
-            });
-            Thread thread = new Thread(() -> {
-                try {
-                    remover.forceRemoveRecursive(dir.toPath());
-                } catch (IOException e) {
-                    removed.set(e);
-                }
-            });
-            thread.start();
-            TimeUnit.MILLISECONDS.sleep(100);
-            thread.interrupt();
-            thread.join();
-            assertFalse(thread.isAlive());
-            assertTrue(d1f1.exists());
-            IOException ioException = removed.get();
-            assertNotNull(ioException);
-            assertThat(ioException.getMessage(), containsString(dir.getPath()));
-            assertNotNull(interrupted.get());
-        }
+        locker.acquireLock(d1f1);
+        AtomicReference<InterruptedException> interrupted = new AtomicReference<>();
+        AtomicReference<IOException> removed = new AtomicReference<>();
+        PathRemover remover = PathRemover.newRemoverWithStrategy(retriesAttempted -> {
+            try {
+                TimeUnit.SECONDS.sleep(retriesAttempted + 1);
+                return true;
+            } catch (InterruptedException e) {
+                interrupted.set(e);
+                return false;
+            }
+        });
+        Thread thread = new Thread(() -> {
+            try {
+                remover.forceRemoveRecursive(dir.toPath());
+            } catch (IOException e) {
+                removed.set(e);
+            }
+        });
+        thread.start();
+        TimeUnit.MILLISECONDS.sleep(100);
+        thread.interrupt();
+        thread.join();
+        assertFalse(thread.isAlive());
+        assertTrue(d1f1.exists());
+        IOException ioException = removed.get();
+        assertNotNull(ioException);
+        assertThat(ioException.getMessage(), containsString(dir.getPath()));
+        assertNotNull(interrupted.get());
     }
 
     private static void mkdirs(File... dirs) {