ACS-2243 Attempt to fix intermittent failure in JobLockServiceTest (#862)

repository/src/test/java/org/alfresco/repo/lock/JobLockServiceTest.java

@@ -25,6 +25,14 @@
  */
 package org.alfresco.repo.lock;
 
+import static java.time.Duration.of;
+import static java.time.temporal.ChronoUnit.MILLIS;
+import static java.time.temporal.ChronoUnit.SECONDS;
+
+import java.time.Duration;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import junit.framework.AssertionFailedError;
 import junit.framework.TestCase;
 
 import org.alfresco.repo.domain.locks.LockDAO;
@@ -37,7 +45,6 @@ import org.alfresco.service.transaction.TransactionService;
 import org.alfresco.test_category.OwnJVMTestsCategory;
 import org.alfresco.util.ApplicationContextHelper;
 import org.alfresco.util.testing.category.DBTests;
-import org.alfresco.util.testing.category.FrequentlyFailingTests;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.apache.log4j.Level;
@@ -508,7 +515,6 @@ public class JobLockServiceTest extends TestCase
     public void testGetLockWithCallbackNullCallback()   { runGetLockWithCallback(1); }
     public void testGetLockWithCallbackShortTTL()       { runGetLockWithCallback(2); }
     public void testGetLockWithCallbackLocked()         { runGetLockWithCallback(3); }
-    @Category(FrequentlyFailingTests.class) // ACS-2243
     public void testGetLockWithCallbackNormal()         { runGetLockWithCallback(4); }
     
     public void runGetLockWithCallback(int t)
@@ -528,7 +534,7 @@ public class JobLockServiceTest extends TestCase
         { 
             QName        lockName   = t==0 ? null : lockA;
             TestCallback callback   = t==1 ? null : new TestCallback();
-            long         timeToLive = t==2 ? 1    : 50; 
+            long         timeToLive = t==2 ? 1    : 500;
 
             if (t==3) 
             {
@@ -543,20 +549,22 @@ public class JobLockServiceTest extends TestCase
             
             if (callback == null) throw new IllegalStateException();
 
+            waitForAssertion(of(100, MILLIS), () -> {
                 assertEquals(false,callback.released);
-            assertEquals(0,callback.isActiveCount);
+                assertEquals(0,callback.getIsActiveCount());
-            
+            });
-            Thread.sleep(40);
 
+            waitForAssertion(of(1, SECONDS), () -> {
                 assertEquals(false, callback.released);
-            assertEquals(1,callback.isActiveCount);
+                assertEquals(1, callback.getIsActiveCount());
+            });
 
             callback.isActive = false;
 
-            Thread.sleep(1000); // need to make this quite long to account for slow database updates
+            waitForAssertion(of(2, SECONDS), () -> {
-            
                 assertEquals(true, callback.released);
-            assertEquals(2,callback.isActiveCount);
+                assertEquals(2, callback.getIsActiveCount());
+            });
         }
         catch (IllegalArgumentException e)
         {            
@@ -614,17 +622,54 @@ public class JobLockServiceTest extends TestCase
         }
     }
 
+    private static void waitForAssertion(Duration timeout, Runnable assertion)
+    {
+        logger.debug("Waiting for assertion to succeed.");
+        final long lastStep = 10;
+        final long delayMillis = timeout.toMillis() > lastStep ? timeout.toMillis() / lastStep : 1;
+
+        for (int s = 0; s <= lastStep; s++)
+        {
+            try
+            {
+                assertion.run();
+                logger.debug("Assertion succeeded.");
+                return;
+            }
+            catch (AssertionFailedError e)
+            {
+                if (s == lastStep)
+                {
+                    logger.debug("Assertion failed. No more waiting.");
+                    throw e;
+                }
+                logger.debug("Assertion failed. Waiting until it succeeds.", e);
+            }
+            try
+            {
+                Thread.sleep(delayMillis);
+            }
+            catch (InterruptedException e)
+            {
+                Thread.currentThread().interrupt();
+                fail("Thread has been interrupted.");
+            }
+        }
+
+        throw new IllegalStateException("Unexpected.");
+    }
+    
     private class TestCallback implements JobLockRefreshCallback
     {
-        public volatile long isActiveCount;
+        public volatile AtomicInteger isActiveCounter = new AtomicInteger();
         public volatile boolean released;
         public volatile boolean isActive = true;
 
         @Override
         public boolean isActive()
         {
-            isActiveCount++;
+            final int currentIsActiveCount = isActiveCounter.incrementAndGet();
-            logger.debug("TestCallback.isActive => "+isActive+" ("+isActiveCount+")");
+            logger.debug("TestCallback.isActive => "+isActive+" ("+currentIsActiveCount+")");
             return isActive;
         }
 
@@ -634,5 +679,10 @@ public class JobLockServiceTest extends TestCase
             logger.debug("TestCallback.lockReleased");
             released = true;
         }
+
+        public int getIsActiveCount()
+        {
+            return isActiveCounter.get();
+        }
     }    
 }