SONAR-5062 New implementation of concurrent safe unzip using FileLock

2 files changed, 22 insertions, 33 deletions

diff --git a/sonar-home/src/main/java/org/sonar/home/cache/FileCache.java b/sonar-home/src/main/java/org/sonar/home/cache/FileCache.java
index a02eefc6d3e..37a81e17fcf 100644
--- a/sonar-home/src/main/java/org/sonar/home/cache/FileCache.java
+++ b/sonar-home/src/main/java/org/sonar/home/cache/FileCache.java
@@ -19,7 +19,6 @@
  */
 package org.sonar.home.cache;
 
-import org.apache.commons.io.FileExistsException;
 import org.apache.commons.io.FileUtils;
 import org.sonar.api.utils.ZipUtils;
 import org.sonar.home.log.Log;
@@ -27,6 +26,7 @@ import org.sonar.home.log.Log;
 import javax.annotation.CheckForNull;
 
 import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.util.Random;
 import java.util.zip.ZipEntry;
@@ -185,16 +185,24 @@ public class FileCache {
   public File unzip(File cachedFile) throws IOException {
     String filename = cachedFile.getName();
     File destDir = new File(cachedFile.getParentFile(), filename + "_unzip");
+    File lockFile = new File(cachedFile.getParentFile(), filename + "_unzip.lock");
     if (!destDir.exists()) {
-      File tempDir = createTempDir();
-      ZipUtils.unzip(cachedFile, tempDir, new LibFilter());
+      FileOutputStream out = new FileOutputStream(lockFile);
       try {
-        // Recheck in case of concurrent processes
-        if (!destDir.exists()) {
-          FileUtils.moveDirectory(tempDir, destDir);
+        java.nio.channels.FileLock lock = out.getChannel().lock();
+        try {
+          // Recheck in case of concurrent processes
+          if (!destDir.exists()) {
+            File tempDir = createTempDir();
+            ZipUtils.unzip(cachedFile, tempDir, new LibFilter());
+            FileUtils.moveDirectory(tempDir, destDir);
+          }
+        } finally {
+          lock.release();
         }
-      } catch (FileExistsException e) {
-        // Ignore as is certainly means a concurrent process has unziped the same file
+      } finally {
+        out.close();
+        FileUtils.deleteQuietly(lockFile);
       }
     }
     return destDir;
diff --git a/sonar-home/src/test/java/org/sonar/home/cache/FileCacheTest.java b/sonar-home/src/test/java/org/sonar/home/cache/FileCacheTest.java
index d5ba2fdbbcd..2b2ee395820 100644
--- a/sonar-home/src/test/java/org/sonar/home/cache/FileCacheTest.java
+++ b/sonar-home/src/test/java/org/sonar/home/cache/FileCacheTest.java
@@ -20,7 +20,6 @@
 package org.sonar.home.cache;
 
 import org.apache.commons.io.FileUtils;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
@@ -30,13 +29,7 @@ import org.sonar.home.log.Slf4jLog;
 import java.io.File;
 import java.io.IOException;
 import java.net.URISyntaxException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-import java.util.concurrent.Future;
 
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Matchers.any;
@@ -129,8 +122,7 @@ public class FileCacheTest {
   }
 
   @Test
-  @Ignore("Implementation is not safe so the test sometimes fails")
-  public void concurrent_unzip_from_cache() throws IOException, URISyntaxException, InterruptedException, ExecutionException {
+  public void unzip_from_cache() throws IOException, URISyntaxException, InterruptedException, ExecutionException {
     final File samplePlugin = new File(this.getClass().getResource("/sonar-checkstyle-plugin-2.8.jar").toURI());
     FileHashes hashes = mock(FileHashes.class);
     final FileCache cache = new FileCache(tempFolder.newFolder(), log, hashes);
@@ -145,27 +137,16 @@ public class FileCacheTest {
     assertThat(cachedFile).isNotNull().exists().isFile();
     assertThat(cachedFile.getName()).isEqualTo("sonar-checkstyle-plugin-2.8.jar");
 
-    final int nThreads = 5;
-    ExecutorService executorService = Executors.newFixedThreadPool(nThreads);
-    List<Callable<File>> tasks = new ArrayList<Callable<File>>();
-    for (int i = 0; i < nThreads; i++) {
-      tasks.add(new Callable<File>() {
-
-        public File call() throws Exception {
-          return cache.unzip(cachedFile);
-        }
-      });
-    }
-
-    File pluginDepsDir = null;
-    for (Future<File> result : executorService.invokeAll(tasks)) {
-      pluginDepsDir = result.get();
-    }
+    File pluginDepsDir = cache.unzip(cachedFile);
+
     assertThat(pluginDepsDir.listFiles()).hasSize(1);
     File metaDir = new File(pluginDepsDir, "META-INF");
     assertThat(metaDir.listFiles()).hasSize(1);
     File libDir = new File(metaDir, "lib");
     assertThat(libDir.listFiles()).hasSize(3);
     assertThat(libDir.listFiles()).containsOnly(new File(libDir, "antlr-2.7.6.jar"), new File(libDir, "checkstyle-5.1.jar"), new File(libDir, "commons-cli-1.0.jar"));
+
+    // Unzip again should not do anything as it is already unzipped
+    cache.unzip(cachedFile);
   }
 }