diff --git a/src/changes/changes.xml b/src/changes/changes.xml index efb78ebee..c08883917 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -22,6 +22,7 @@ + Remove busy wait from AtomicSafeInitializer.get() DiffBuilder.append(String, Object left, Object right) does not do a left.equals(right) check StrSubstitutor.replaceSystemProperties does not work consistently Add option to disable the "objectsTriviallyEqual" test in DiffBuilder diff --git a/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java b/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java index 5bb5096db..a1186dba2 100644 --- a/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java +++ b/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java @@ -16,6 +16,7 @@ */ package org.apache.commons.lang3.concurrent; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicReference; /** @@ -62,20 +63,44 @@ public abstract class AtomicSafeInitializer implements /** Holds the reference to the managed object. */ private final AtomicReference reference = new AtomicReference(); + /** Holds the exception that terminated the initialize() method, if an exception was thrown */ + private final AtomicReference referenceExc = new AtomicReference(); + + /** Latch for those threads waiting for initialization to complete. */ + private final CountDownLatch latch = new CountDownLatch(1); + /** * Get (and initialize, if not initialized yet) the required object * * @return lazily initialized object * @throws ConcurrentException if the initialization of the object causes an - * exception + * exception or the thread is interrupted waiting for another thread to + * complete the initialization */ @Override public final T get() throws ConcurrentException { T result; - while ((result = reference.get()) == null) { + if ((result = reference.get()) == null) { if (factory.compareAndSet(null, this)) { - reference.set(initialize()); + try { + reference.set(result = initialize()); + } catch ( ConcurrentException exc ) { + referenceExc.set(exc); + throw exc; + } finally { + latch.countDown(); + } + } else { + try { + latch.await(); + if ( referenceExc.get() != null ) { + throw new ConcurrentException(referenceExc.get().getMessage(), referenceExc.get().getCause()); + } + result = reference.get(); + } catch (InterruptedException intExc) { + throw new ConcurrentException("interrupted waiting for initialization to complete", intExc); + } } } diff --git a/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java b/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java index 6bd8204af..d3fd4c129 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurrent; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; import java.util.concurrent.CountDownLatch; @@ -72,7 +74,41 @@ public abstract class AbstractConcurrentInitializerTest { @Test public void testGetConcurrent() throws ConcurrentException, InterruptedException { - final ConcurrentInitializer initializer = createInitializer(); + + this.testGetConcurrentOptionallyWithException(false, null, null); + } + + /** + * Tests the handling of exceptions thrown on the initialized when multiple threads execute concurrently. + * Always an exception with the same message and cause should be thrown. + * + * @throws org.apache.commons.lang3.concurrent.ConcurrentException because the object under test may throw it + * @throws java.lang.InterruptedException because the threading API my throw it + */ + public void testGetConcurrentWithException(String expectedMessage, + Exception expectedCause) + throws ConcurrentException, InterruptedException { + + this.testGetConcurrentOptionallyWithException(true, expectedMessage, expectedCause); + } + + /** + * Tests whether get() can be invoked from multiple threads concurrently. Supports the exception-handling case + * and the normal, non-exception case. + * + * Always the same object should be returned, or an exception with the same message and cause should be thrown. + * + * @throws org.apache.commons.lang3.concurrent.ConcurrentException because the object under test may throw it + * @throws java.lang.InterruptedException because the threading API my throw it + */ + protected void testGetConcurrentOptionallyWithException(boolean expectExceptions, String expectedMessage, + Exception expectedCause) + throws ConcurrentException, InterruptedException { + + final ConcurrentInitializer initializer = expectExceptions ? + createExceptionThrowingInitializer() : + createInitializer(); + final int threadCount = 20; final CountDownLatch startLatch = new CountDownLatch(1); class GetThread extends Thread { @@ -106,9 +142,18 @@ public abstract class AbstractConcurrentInitializerTest { } // check results - final Object managedObject = initializer.get(); - for (final GetThread t : threads) { - assertEquals("Wrong object", managedObject, t.object); + if ( expectExceptions ) { + for (GetThread t : threads) { + assertTrue(t.object instanceof Exception); + Exception exc = (Exception) t.object; + assertEquals(expectedMessage, exc.getMessage()); + assertSame(expectedCause, exc.getCause()); + } + } else { + final Object managedObject = initializer.get(); + for (final GetThread t : threads) { + assertEquals("Wrong object", managedObject, t.object); + } } } @@ -119,4 +164,12 @@ public abstract class AbstractConcurrentInitializerTest { * @return the initializer object to be tested */ protected abstract ConcurrentInitializer createInitializer(); + + /** + * Creates a {@link ConcurrentInitializer} object that always throws + * exceptions. + * + * @return + */ + protected abstract ConcurrentInitializer createExceptionThrowingInitializer(); } diff --git a/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java b/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java index 7434bb5dc..9aa83fcd6 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java @@ -16,12 +16,29 @@ */ package org.apache.commons.lang3.concurrent; +import org.junit.Test; + /** * Test class for {@code AtomicInitializer}. * * @version $Id$ */ public class AtomicInitializerTest extends AbstractConcurrentInitializerTest { + private Exception testCauseException; + private String testExceptionMessage; + + public AtomicInitializerTest() { + testExceptionMessage = "x-test-exception-message-x"; + testCauseException = new Exception(testExceptionMessage); + } + + @Test + public void testGetConcurrentWithException () + throws ConcurrentException, InterruptedException { + + super.testGetConcurrentWithException(testExceptionMessage, testCauseException); + } + /** * Returns the initializer to be tested. * @@ -36,4 +53,20 @@ public class AtomicInitializerTest extends AbstractConcurrentInitializerTest { } }; } + + @Override + protected ConcurrentInitializer createExceptionThrowingInitializer() { + return new ExceptionThrowingAtomicSafeInitializerTestImpl(); + } + + /** + * A concrete test implementation of {@code AtomicSafeInitializer}. This + * implementation always throws an exception. + */ + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends AtomicSafeInitializer { + @Override + protected Object initialize() throws ConcurrentException { + throw new ConcurrentException(testExceptionMessage, testCauseException); + } + } } diff --git a/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java b/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java index bcf1688cb..27f36c7e8 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java @@ -17,7 +17,11 @@ package org.apache.commons.lang3.concurrent; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.atomic.AtomicInteger; import org.junit.Before; @@ -30,12 +34,19 @@ import org.junit.Test; */ public class AtomicSafeInitializerTest extends AbstractConcurrentInitializerTest { + /** The instance to be tested. */ private AtomicSafeInitializerTestImpl initializer; + private ExceptionThrowingAtomicSafeInitializerTestImpl exceptionThrowingInitializer; + private Exception testCauseException; + private String testExceptionMessage; @Before public void setUp() throws Exception { initializer = new AtomicSafeInitializerTestImpl(); + exceptionThrowingInitializer = new ExceptionThrowingAtomicSafeInitializerTestImpl(); + testExceptionMessage = "x-test-exception-message-x"; + testCauseException = new Exception(testExceptionMessage); } /** @@ -48,6 +59,17 @@ public class AtomicSafeInitializerTest extends return initializer; } + /** + * Returns the exception-throwing initializer to be tested. + * + * @return the {@code AtomicSafeInitializer} under test when validating + * exception handling + */ + @Override + protected ConcurrentInitializer createExceptionThrowingInitializer() { + return exceptionThrowingInitializer; + } + /** * Tests that initialize() is called only once. * @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest extends initializer.initCounter.get()); } + @Test + public void testExceptionOnInitialize() throws ConcurrentException, + InterruptedException { + + testGetConcurrentWithException(testExceptionMessage, testCauseException); + } + + /** + * Validate the handling of an interrupted exception on a thread waiting for another thread to finish calling the + * initialize() method. + * + * @throws Exception + */ + @Test(timeout = 3000) + public void testInterruptedWaitingOnInitialize() throws Exception { + this.execTestWithWaitingOnInitialize(true); + } + + /** + * Test the success path of two threads reaching the initialization point at the same time. + */ + @Test(timeout = 3000) + public void testOneThreadWaitingForAnotherToInitialize () throws Exception { + execTestWithWaitingOnInitialize(false); + } + + + /** + * Execute a test that requires one thread to be waiting on the initialize() method of another thread. This test + * uses latches to guarantee the code path being tested. + * + * @throws Exception + */ + protected void execTestWithWaitingOnInitialize(boolean interruptInd) throws Exception { + final CountDownLatch startLatch = new CountDownLatch(1); + final CountDownLatch finishLatch = new CountDownLatch(1); + final WaitingInitializerTestImpl initializer = new WaitingInitializerTestImpl(startLatch, finishLatch); + + InitializerTestThread execThread1 = new InitializerTestThread(initializer); + InitializerTestThread execThread2 = new InitializerTestThread(initializer); + + // Start the first thread and wait for it to get into the initialize method so we are sure it is the thread + // executing initialize(). + execThread1.start(); + startLatch.await(); + + // Start the second thread and interrupt it to force the InterruptedException. There is no need to make sure + // the thread reaches the await() call before interrupting it. + execThread2.start(); + + if ( interruptInd ) { + // Interrupt the second thread now and wait for it to complete to ensure it reaches the wait inside the + // get() method. + execThread2.interrupt(); + execThread2.join(); + } + + // Signal the completion of the initialize method now. + finishLatch.countDown(); + + // Wait for the initialize() to finish. + execThread1.join(); + + // Wait for thread2 to finish, if it was not already done + if ( ! interruptInd ) { + execThread2.join(); + } + + // + // Validate: thread1 should have the valid result; thread2 should have caught an interrupted exception, if + // interrupted, or should have the same result otherwise. + // + assertFalse(execThread1.isCaughtException()); + assertSame(initializer.getAnswer(), execThread1.getResult()); + + if ( interruptInd ) { + assertTrue(execThread2.isCaughtException()); + Exception exc = (Exception) execThread2.getResult(); + assertTrue(exc.getCause() instanceof InterruptedException); + assertEquals("interrupted waiting for initialization to complete", exc.getMessage()); + } else { + assertFalse(execThread2.isCaughtException()); + assertSame(initializer.getAnswer(), execThread2.getResult()); + } + } + /** * A concrete test implementation of {@code AtomicSafeInitializer}. This * implementation also counts the number of invocations of the initialize() @@ -78,4 +186,90 @@ public class AtomicSafeInitializerTest extends return new Object(); } } + + /** + * A concrete test implementation of {@code AtomicSafeInitializer}. This + * implementation always throws an exception. + */ + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends AtomicSafeInitializer { + @Override + protected Object initialize() throws ConcurrentException { + throw new ConcurrentException(testExceptionMessage, testCauseException); + } + } + + /** + * Initializer that signals it has started and waits to complete until signalled in order to enable a guaranteed + * order-of-operations. This allows the test code to peg one thread to the initialize method for a period of time + * that the test can dictate. + */ + private class WaitingInitializerTestImpl extends AtomicSafeInitializer { + private final CountDownLatch startedLatch; + private final CountDownLatch finishLatch; + private final Object answer = new Object(); + + public WaitingInitializerTestImpl(CountDownLatch startedLatch, CountDownLatch finishLatch) { + this.startedLatch = startedLatch; + this.finishLatch = finishLatch; + } + + @Override + protected Object initialize() throws ConcurrentException { + this.startedLatch.countDown(); + try { + this.finishLatch.await(); + } catch (InterruptedException intExc) { + throw new ConcurrentException(intExc); + } + + return answer; + } + + public Object getAnswer () { + return answer; + } + } + + /** + * Test executor of the initializer get() operation that captures the result. + */ + private class InitializerTestThread extends Thread { + private AtomicSafeInitializer initializer; + private Object result; + private boolean caughtException; + + public InitializerTestThread(AtomicSafeInitializer initializer) { + super("AtomicSafeInitializer test thread"); + this.initializer = initializer; + } + + @Override + public void run() { + try { + this.result = initializer.get(); + } catch ( ConcurrentException concurrentExc ) { + this.caughtException = true; + this.result = concurrentExc; + } + } + + /** + * Resulting object, if the get() method returned successfully, or exception if an exception was thrown. + * + * @return resulting object or exception from the get() method call. + */ + public Object getResult () { + return this.result; + } + + /** + * Determine whether an exception was caught on the get() call. Does not guarantee that the get() method was + * called or completed. + * + * @return true => exception was caught; false => exception was not caught. + */ + public boolean isCaughtException () { + return this.caughtException; + } + } } diff --git a/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java b/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java index 409234ff1..45ca35d1f 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java @@ -17,6 +17,7 @@ package org.apache.commons.lang3.concurrent; import org.junit.Before; +import org.junit.Test; /** * Test class for {@code LazyInitializer}. @@ -26,10 +27,16 @@ import org.junit.Before; public class LazyInitializerTest extends AbstractConcurrentInitializerTest { /** The initializer to be tested. */ private LazyInitializerTestImpl initializer; + private ExceptionThrowingLazyInitializerTestImpl exceptionThrowingInitializer; + private Exception testCauseException; + private String testExceptionMessage; @Before public void setUp() throws Exception { initializer = new LazyInitializerTestImpl(); + exceptionThrowingInitializer = new ExceptionThrowingLazyInitializerTestImpl(); + testExceptionMessage = "x-test-exception-message-x"; + testCauseException = new Exception(testExceptionMessage); } /** @@ -43,6 +50,18 @@ public class LazyInitializerTest extends AbstractConcurrentInitializerTest { return initializer; } + @Override + protected ConcurrentInitializer createExceptionThrowingInitializer() { + return exceptionThrowingInitializer; + } + + @Test + public void testGetConcurrentWithException () + throws ConcurrentException, InterruptedException { + + super.testGetConcurrentWithException(testExceptionMessage, testCauseException); + } + /** * A test implementation of LazyInitializer. This class creates a plain * Object. As Object does not provide a specific equals() method, it is easy @@ -55,4 +74,16 @@ public class LazyInitializerTest extends AbstractConcurrentInitializerTest { return new Object(); } } + + + /** + * A concrete test implementation of {@code AtomicSafeInitializer}. This + * implementation always throws an exception. + */ + private class ExceptionThrowingLazyInitializerTestImpl extends LazyInitializer { + @Override + protected Object initialize() throws ConcurrentException { + throw new ConcurrentException(testExceptionMessage, testCauseException); + } + } }