From: Sébastien Lesaint <sebastien.lesaint@sonarsource.com>
Date: Wed, 22 Apr 2015 12:04:55 +0000 (+0200)
Subject: SONAR-6366 improve handling of really simultaneous db migration calls
X-Git-Tag: 5.2-RC1~2143
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=4847890d7a020dcd278863e0ac7484253429c4b8;p=sonarqube.git

SONAR-6366 improve handling of really simultaneous db migration calls

this will also make unit test stable
---

diff --git a/server/sonar-server/src/main/java/org/sonar/server/db/migrations/PlatformDatabaseMigration.java b/server/sonar-server/src/main/java/org/sonar/server/db/migrations/PlatformDatabaseMigration.java
index 3425c284e92..f7fc0ad2060 100644
--- a/server/sonar-server/src/main/java/org/sonar/server/db/migrations/PlatformDatabaseMigration.java
+++ b/server/sonar-server/src/main/java/org/sonar/server/db/migrations/PlatformDatabaseMigration.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.server.db.migrations;
 
+import org.sonar.api.utils.log.Logger;
 import org.sonar.api.utils.log.Loggers;
 import org.sonar.server.ruby.RubyBridge;
 
@@ -33,6 +34,8 @@ import java.util.concurrent.locks.ReentrantLock;
  */
 public class PlatformDatabaseMigration implements DatabaseMigration {
 
+  private static final Logger LOGGER = Loggers.get(PlatformDatabaseMigration.class);
+
   private final RubyBridge rubyBridge;
   /**
    * ExecutorService implements threads management.
@@ -50,7 +53,7 @@ public class PlatformDatabaseMigration implements DatabaseMigration {
    * by the thread executing the db migration.
    * </p>
    */
-  private AtomicBoolean running = new AtomicBoolean(false);
+  private final AtomicBoolean running = new AtomicBoolean(false);
   private Status status = Status.NONE;
   @Nullable
   private Date startDate;
@@ -64,15 +67,17 @@ public class PlatformDatabaseMigration implements DatabaseMigration {
 
   @Override
   public void startIt() {
-    if (lock.isLocked() || this.running.get() /* fail-fast if db migration is running */) {
+    if (lock.isLocked() || this.running.get()) {
+      LOGGER.trace("{}: lock is already taken or process is already running", Thread.currentThread().getName());
       return;
     }
 
-    lock.lock();
-    try {
-      startAsynchronousDBMigration();
-    } finally {
-      lock.unlock();
+    if (lock.tryLock()) {
+      try {
+        startAsynchronousDBMigration();
+      } finally {
+        lock.unlock();
+      }
     }
   }
 
@@ -92,12 +97,12 @@ public class PlatformDatabaseMigration implements DatabaseMigration {
         startDate = new Date();
         failureError = null;
         try {
-          Loggers.get(PlatformDatabaseMigration.class).info("Starting DB Migration at {}", startDate);
+          LOGGER.info("Starting DB Migration at {}", startDate);
           rubyBridge.databaseMigration().trigger();
-          Loggers.get(PlatformDatabaseMigration.class).info("DB Migration ended successfully at {}", new Date());
+          LOGGER.info("DB Migration ended successfully at {}", new Date());
           status = Status.SUCCEEDED;
         } catch (Throwable t) {
-          Loggers.get(PlatformDatabaseMigration.class).error("DB Migration failed and ended at " + startDate + " with an exception", t);
+          LOGGER.error("DB Migration failed and ended at " + startDate + " with an exception", t);
           status = Status.FAILED;
           failureError = t;
         } finally {
diff --git a/server/sonar-server/src/test/java/org/sonar/server/db/migrations/PlatformDatabaseMigrationConcurrentAccessTest.java b/server/sonar-server/src/test/java/org/sonar/server/db/migrations/PlatformDatabaseMigrationConcurrentAccessTest.java
index 6f6cfa1b3dd..09d00c5695f 100644
--- a/server/sonar-server/src/test/java/org/sonar/server/db/migrations/PlatformDatabaseMigrationConcurrentAccessTest.java
+++ b/server/sonar-server/src/test/java/org/sonar/server/db/migrations/PlatformDatabaseMigrationConcurrentAccessTest.java
@@ -21,50 +21,58 @@ package org.sonar.server.db.migrations;
 
 import com.google.common.base.Throwables;
 import org.junit.After;
-import org.junit.Before;
 import org.junit.Test;
-import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
 import org.sonar.server.ruby.RubyBridge;
 import org.sonar.server.ruby.RubyDatabaseMigration;
 
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
-import static org.mockito.internal.verification.VerificationModeFactory.times;
 
 public class PlatformDatabaseMigrationConcurrentAccessTest {
 
   private ExecutorService pool = Executors.newFixedThreadPool(2);
   /**
-   * Implementation of execute runs Runnable synchronously with a delay of 200ms.
+   * Latch is used to make sure both testing threads try and call {@link PlatformDatabaseMigration#startIt()} at the
+   * same time
+   */
+  private CountDownLatch latch = new CountDownLatch(2);
+
+  /**
+   * Implementation of execute runs Runnable synchronously
    */
   private PlatformDatabaseMigrationExecutorService executorService = new PlatformDatabaseMigrationExecutorServiceAdaptor() {
     @Override
     public void execute(Runnable command) {
+      command.run();
+    }
+  };
+  /**
+   * thread-safe counter of calls to the trigger method of {@link #rubyDatabaseMigration}
+   */
+  private AtomicInteger triggerCount = new AtomicInteger();
+  /**
+   * Implementation of RubyDatabaseMigration which trigger method increments a thread-safe counter and add a delay of 200ms
+   */
+  private RubyDatabaseMigration rubyDatabaseMigration = new RubyDatabaseMigration() {
+    @Override
+    public void trigger() {
+      triggerCount.incrementAndGet();
       try {
         Thread.currentThread().sleep(200);
       } catch (InterruptedException e) {
         Throwables.propagate(e);
       }
-      command.run();
     }
   };
-  @Mock
-  private RubyDatabaseMigration rubyDatabaseMigration;
-  @Mock
-  private RubyBridge rubyBridge;
-  private PlatformDatabaseMigration underTest;
-
-  @Before
-  public void setUp() throws Exception {
-    MockitoAnnotations.initMocks(this);
-    underTest = new PlatformDatabaseMigration(rubyBridge, executorService);
-  }
+  private RubyBridge rubyBridge = mock(RubyBridge.class);
+  private PlatformDatabaseMigration underTest = new PlatformDatabaseMigration(rubyBridge, executorService);
 
   @After
   public void tearDown() throws Exception {
@@ -80,14 +88,19 @@ public class PlatformDatabaseMigrationConcurrentAccessTest {
 
     pool.awaitTermination(3, TimeUnit.SECONDS);
 
-    verify(rubyBridge, times(1)).databaseMigration();
-
-    assertThat(underTest.status()).isEqualTo(DatabaseMigration.Status.SUCCEEDED);
+    assertThat(triggerCount.get()).isEqualTo(1);
   }
 
   private class CallStartit implements Runnable {
     @Override
     public void run() {
+      latch.countDown();
+      try {
+        latch.await();
+      } catch (InterruptedException e) {
+        // propagate interruption
+        Thread.currentThread().interrupt();
+      }
       underTest.startIt();
     }
   }