You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by br...@apache.org on 2015/02/23 21:15:50 UTC
svn commit: r1661762 - in /commons/proper/lang/trunk/src: changes/
main/java/org/apache/commons/lang3/concurrent/
test/java/org/apache/commons/lang3/concurrent/
Author: britter
Date: Mon Feb 23 20:15:49 2015
New Revision: 1661762
URL: http://svn.apache.org/r1661762
Log:
LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also fixes #46 from github. Thanks to github user artnaseef.
Modified:
commons/proper/lang/trunk/src/changes/changes.xml
commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
Modified: commons/proper/lang/trunk/src/changes/changes.xml
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
+++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23 20:15:49 2015
@@ -22,6 +22,7 @@
<body>
<release version="3.4" date="tba" description="tba">
+ <action issue="LANG-1086" type="update" dev="britter">Remove busy wait from AtomicSafeInitializer.get()</action>
<action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan Baker">DiffBuilder.append(String, Object left, Object right) does not do a left.equals(right) check</action>
<action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan Baker">StrSubstitutor.replaceSystemProperties does not work consistently</action>
<action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan Baker">Add option to disable the "objectsTriviallyEqual" test in DiffBuilder</action>
Modified: commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java (original)
+++ commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java Mon Feb 23 20:15:49 2015
@@ -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 AtomicSafeInitiali
/** Holds the reference to the managed object. */
private final AtomicReference<T> reference = new AtomicReference<T>();
+ /** Holds the exception that terminated the initialize() method, if an exception was thrown */
+ private final AtomicReference<ConcurrentException> referenceExc = new AtomicReference<ConcurrentException>();
+
+ /** 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);
+ }
}
}
Modified: commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java (original)
+++ commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java Mon Feb 23 20:15:49 2015
@@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
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 AbstractConcurrent
@Test
public void testGetConcurrent() throws ConcurrentException,
InterruptedException {
- final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
}
// 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 AbstractConcurrent
* @return the initializer object to be tested
*/
protected abstract ConcurrentInitializer<Object> createInitializer();
+
+ /**
+ * Creates a {@link ConcurrentInitializer} object that always throws
+ * exceptions.
+ *
+ * @return
+ */
+ protected abstract ConcurrentInitializer<Object> createExceptionThrowingInitializer();
}
Modified: commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java (original)
+++ commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java Mon Feb 23 20:15:49 2015
@@ -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 exten
}
};
}
+
+ @Override
+ protected ConcurrentInitializer<Object> createExceptionThrowingInitializer() {
+ return new ExceptionThrowingAtomicSafeInitializerTestImpl();
+ }
+
+ /**
+ * A concrete test implementation of {@code AtomicSafeInitializer}. This
+ * implementation always throws an exception.
+ */
+ private class ExceptionThrowingAtomicSafeInitializerTestImpl extends AtomicSafeInitializer<Object> {
+ @Override
+ protected Object initialize() throws ConcurrentException {
+ throw new ConcurrentException(testExceptionMessage, testCauseException);
+ }
+ }
}
Modified: commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java (original)
+++ commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java Mon Feb 23 20:15:49 2015
@@ -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);
}
/**
@@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
}
/**
+ * Returns the exception-throwing initializer to be tested.
+ *
+ * @return the {@code AtomicSafeInitializer} under test when validating
+ * exception handling
+ */
+ @Override
+ protected ConcurrentInitializer<Object> createExceptionThrowingInitializer() {
+ return exceptionThrowingInitializer;
+ }
+
+ /**
* Tests that initialize() is called only once.
*
* @throws org.apache.commons.lang3.concurrent.ConcurrentException because {@link #testGetConcurrent()} may throw it
@@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
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 e
return new Object();
}
}
+
+ /**
+ * A concrete test implementation of {@code AtomicSafeInitializer}. This
+ * implementation always throws an exception.
+ */
+ private class ExceptionThrowingAtomicSafeInitializerTestImpl extends AtomicSafeInitializer<Object> {
+ @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<Object> {
+ 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<Object> initializer;
+ private Object result;
+ private boolean caughtException;
+
+ public InitializerTestThread(AtomicSafeInitializer<Object> 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;
+ }
+ }
}
Modified: commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
URL: http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
==============================================================================
--- commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java (original)
+++ commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java Mon Feb 23 20:15:49 2015
@@ -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
return initializer;
}
+ @Override
+ protected ConcurrentInitializer<Object> 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
return new Object();
}
}
+
+
+ /**
+ * A concrete test implementation of {@code AtomicSafeInitializer}. This
+ * implementation always throws an exception.
+ */
+ private class ExceptionThrowingLazyInitializerTestImpl extends LazyInitializer<Object> {
+ @Override
+ protected Object initialize() throws ConcurrentException {
+ throw new ConcurrentException(testExceptionMessage, testCauseException);
+ }
+ }
}
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src: changes/
main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Oliver Heger <ol...@oliver-heger.de>.
Hi Arthur,
many thanks for this detailed analysis! Your conclusion is to the point:
your proposal is certainly valuable, but it is hard to explain it to
users and give them sound recommendations. So I am on the fence. If we
add this class, the pros and cons you listed should in any case become
part of the Javadocs.
Benedikt suggested having some micro benchmarks comparing the different
initializer implementations. It would be great for the user guide to
have a table listing the behavior of the initializers for different
problem sizes (i.e. the time required for the initialization) and
contention. However, I am afraid it will be really hard to construct a
valid test scenario producing meaningful results.
WDYT?
Oliver
Am 07.03.2015 um 19:02 schrieb Arthur Naseef:
> Great feedback, thank you.
>
> Looking carefully at the operation of LazyInitializer, and the new code,
> they are very similar, and I can see an argument that the new code is not a
> significant change. Let me list key differences, summarize the benefits of
> the new code, and then open the question of next steps.
>
> - LazyInitializer holds a JVM lock over the initialize() method call
> - The new code does not hold a lock over the initialize() method call
> - It is possible to create a JVM deadlock with LazyInitializer (I can
> provide an example)
> - It is possible to create the same deadlock with the new code, but not
> as a JVM deadlock
> - it's possible to break the deadlock without restarting the JVM
> - the deadlock is created from having two threads both waiting
> indefinitely for the other to notify it
> - On exception from the initialize() method
> - LazyInitializer will result in additional calls to the
> initialize() method
> (i.e. the guarantee for LazyInitializer is at most one *successful*
> call to initialize())
> - The new code will refuse to call initialize() again, instead
> throwing a ConcurrentException with the matching message and cause as
> the ConcurrentException resulting from the original call.
> - The new code implements an interruptible wait; it allows threads, that
> are waiting for another thread to finish calling initialize(), to abort
> when interrupted.
>
> Focusing only on the new code, here is how it operates:
>
> - Guarantees initialize() is called at most one time as long as it
> returns a non-null value (null return is a violation of the contract - i.e.
> the results are non-deterministic)
> - Guarantees initialize() is called with no locks held by the initializer
> - Guarantees that all callers receive the same result
> - The initialized object in the success case
> - A ConcurrentException wrapping the root-cause exception thrown by
> the initialize() method
> - Allows subsequent threads waiting on the initialize() to complete to
> be interrupted to abort the wait.
>
> At the heart of the locking issue is one's comfort with JVM locks versus an
> interruptible blocking wait.
>
> So there is possible value here, but I see the argument that this code is
> very similar in operation to LazyInitializer. While avoiding a JVM
> deadlock can be useful, the operation of both sets of code is very similar
> here. Can an application recover from a deadlock with the new code? Yes.
> Will an application do so? That's unclear. Is it advisable to rely on the
> ability to interrupt out of the deadlock? No. In that case, the root
> cause of a deadlock needs to be eliminated. In that regard, the JVM
> deadlock is a little simpler because jstack (and other tools) will clearly
> identify and notify the cause of the deadlock. Could there be other
> reasons to want to interrupt the wait? Yes.
>
> At this point, I'm really starting to argue the nuances and true
> value-to-cost ratio of a concurrent class of this nature. I personally am
> very comfortable with the JVM concurrency primitives, and am aware that
> tools that attempt to simplify the problem for developers can mislead them
> into a false sense of security (e.g. LazyInitializer can easily be involved
> in a deadlock).
>
> Let me know your thoughts. This new implementation can be a valuable
> addition, but this is not a simple matter for users.
>
> Art
>
> P.S. Sorry for the slow response.
>
>
> On Thu, Feb 26, 2015 at 1:29 PM, Oliver Heger <ol...@oliver-heger.de>
> wrote:
>
>> Hi Arthur,
>>
>> I don't have any principle objections against your implementation.
>>
>> I just don't want it as a replacement for the AtomicSafeInitializer
>> class. The latter was designed and announces itself to use only atomic
>> variables for its synchronization and no other types of locks. This
>> premise no longer holds. If the implementation is broken and does not
>> work as expected, we should deprecate and later remove it.
>>
>> What I would be interested in is a comparison between your
>> implementation and LazyInitializer. How do these classes behave in
>> typical scenarios and which one is the preferred option under which
>> conditions?
>>
>> If we add your proposal as an additional initializer implementation, we
>> will also have to update the user's guide and give our users appropriate
>> recommendations when they should use which class.
>>
>> And, we will have to pick a meaningful name for your class.
>>
>> Oliver
>>
>> Am 26.02.2015 um 20:51 schrieb Arthur Naseef:
>>> A few questions coming to mind here:
>>>
>>> - First off, how do we move this forward?
>>> - What do we do with the new implementation?
>>> - What is a good way to address the busy-wait in the original code?
>>> - What is a good way to address the fact that all threads will
>>> busy-wait in the original code if the initialize() method throws an
>>> exception?
>>> - What is a good way to address the original comments which
>> indicate
>>> this class will perform better when, in fact, the busy waiting may
>> have a
>>> significant impact on performance?
>>> - How can we address the fears? Honestly, I think the comment is
>> less a
>>> "fear" than a belief, so how do we prove/disprove?
>>> - What is this other initializer implementation?
>>> - What are the differences in operation between the two?
>>>
>>> Oliver - I'm reading a lot of concerns here, but not seeing how you would
>>> like to address those concerns. Please help me on that front.
>>>
>>> Note, by the way, that the new implementation operates the same as the
>>> original code, to the application using it, except in the case of an
>>> exception thrown on the initialize() call, which is problematic now.
>> That
>>> is, this new implementation guarantees initialize() will only ever be
>>> called one time, and it ensures all callers receive the result of that
>>> initialize() method.
>>>
>>> Art
>>>
>>>
>>>
>>> On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <
>> oliver.heger@oliver-heger.de>
>>> wrote:
>>>
>>>>
>>>>
>>>> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
>>>>> Oliver Heger has raised concerns about this commit in JIRA [1]:
>>>>>
>>>>>> This is a strong change in the behavior of this class. The main
>> property
>>>>> of atomic initializers was that they are non
>>>>>> blocking. Now a blocking wait has been introduced. When there is so
>> much
>>>>> contention that the busy wait is
>>>>>> actually a problem, wouln't it then be better to directly use a
>> blocking
>>>>> variant like lazy initializer?
>>>>>
>>>>> I've looked through the JavaDoc of AtomicInitializer once more. It
>> says:
>>>>> "Because {@code AtomicSafeInitializer} does not use synchronization at
>>>> all
>>>>> it probably outruns {@link LazyInitializer}, at least under low or
>>>> moderate
>>>>> concurrent access."
>>>>>
>>>>> This is the only thing I can find regarding concurrency properties of
>>>>> AtomicInitializer. I think this still holds, doesn't it?
>>>>
>>>> No, the CountDownLatch is synchronized.
>>>>
>>>> There are multiple initializer implementations with different properties
>>>> which are suitable for different use cases and scenarios. The atomic
>>>> initializers are useful if you want to avoid blocking calls, but they
>>>> might be an inferior solution under high contention.
>>>>
>>>> My fear is that this commit optimizes the class for a use case which can
>>>> be served better by another initializer implementation which is already
>>>> blocking; and this optimization has negative impact on the original
>>>> intention of AtomicSafeInitializer.
>>>>
>>>> Oliver
>>>>
>>>>>
>>>>> Benedikt
>>>>>
>>>>> [1] https://issues.apache.org/jira/browse/LANG-1086
>>>>>
>>>>> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
>>>>>
>>>>>> Author: britter
>>>>>> Date: Mon Feb 23 20:15:49 2015
>>>>>> New Revision: 1661762
>>>>>>
>>>>>> URL: http://svn.apache.org/r1661762
>>>>>> Log:
>>>>>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This
>> also
>>>>>> fixes #46 from github. Thanks to github user artnaseef.
>>>>>>
>>>>>> Modified:
>>>>>> commons/proper/lang/trunk/src/changes/changes.xml
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>>
>>>>>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8]
>> (original)
>>>>>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb
>> 23
>>>>>> 20:15:49 2015
>>>>>> @@ -22,6 +22,7 @@
>>>>>> <body>
>>>>>>
>>>>>> <release version="3.4" date="tba" description="tba">
>>>>>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
>>>>>> wait from AtomicSafeInitializer.get()</action>
>>>>>> <action issue="LANG-1081" type="fix" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">DiffBuilder.append(String, Object left, Object right) does not
>>>> do a
>>>>>> left.equals(right) check</action>
>>>>>> <action issue="LANG-1055" type="fix" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">StrSubstitutor.replaceSystemProperties does not work
>>>>>> consistently</action>
>>>>>> <action issue="LANG-1082" type="add" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">Add option to disable the "objectsTriviallyEqual" test in
>>>>>> DiffBuilder</action>
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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 AtomicSafeInitiali
>>>>>> /** Holds the reference to the managed object. */
>>>>>> private final AtomicReference<T> reference = new
>>>> AtomicReference<T>();
>>>>>>
>>>>>> + /** Holds the exception that terminated the initialize() method,
>> if
>>>>>> an exception was thrown */
>>>>>> + private final AtomicReference<ConcurrentException> referenceExc =
>>>> new
>>>>>> AtomicReference<ConcurrentException>();
>>>>>> +
>>>>>> + /** 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);
>>>>>> + }
>>>>>> }
>>>>>> }
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>>>>>>
>>>>>> 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 AbstractConcurrent
>>>>>> @Test
>>>>>> public void testGetConcurrent() throws ConcurrentException,
>>>>>> InterruptedException {
>>>>>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
>>>>>> }
>>>>>>
>>>>>> // 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 AbstractConcurrent
>>>>>> * @return the initializer object to be tested
>>>>>> */
>>>>>> protected abstract ConcurrentInitializer<Object>
>>>> createInitializer();
>>>>>> +
>>>>>> + /**
>>>>>> + * Creates a {@link ConcurrentInitializer} object that always
>>>> throws
>>>>>> + * exceptions.
>>>>>> + *
>>>>>> + * @return
>>>>>> + */
>>>>>> + protected abstract ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer();
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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 exten
>>>>>> }
>>>>>> };
>>>>>> }
>>>>>> +
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer() {
>>>>>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
>>>>>> + }
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>>>> extends
>>>>>> AtomicSafeInitializer<Object> {
>>>>>> + @Override
>>>>>> + protected Object initialize() throws ConcurrentException {
>>>>>> + throw new ConcurrentException(testExceptionMessage,
>>>>>> testCauseException);
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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);
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> + * Returns the exception-throwing initializer to be tested.
>>>>>> + *
>>>>>> + * @return the {@code AtomicSafeInitializer} under test when
>>>>>> validating
>>>>>> + * exception handling
>>>>>> + */
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer() {
>>>>>> + return exceptionThrowingInitializer;
>>>>>> + }
>>>>>> +
>>>>>> + /**
>>>>>> * Tests that initialize() is called only once.
>>>>>> *
>>>>>> * @throws
>> org.apache.commons.lang3.concurrent.ConcurrentException
>>>>>> because {@link #testGetConcurrent()} may throw it
>>>>>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
>>>>>> 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 e
>>>>>> return new Object();
>>>>>> }
>>>>>> }
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>>>> extends
>>>>>> AtomicSafeInitializer<Object> {
>>>>>> + @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<Object> {
>>>>>> + 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<Object> initializer;
>>>>>> + private Object result;
>>>>>> + private boolean caughtException;
>>>>>> +
>>>>>> + public InitializerTestThread(AtomicSafeInitializer<Object>
>>>>>> 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;
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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
>>>>>> return initializer;
>>>>>> }
>>>>>>
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> 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
>>>>>> return new Object();
>>>>>> }
>>>>>> }
>>>>>> +
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingLazyInitializerTestImpl extends
>>>>>> LazyInitializer<Object> {
>>>>>> + @Override
>>>>>> + protected Object initialize() throws ConcurrentException {
>>>>>> + throw new ConcurrentException(testExceptionMessage,
>>>>>> testCauseException);
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Arthur Naseef <ar...@amlinv.com>.
Great feedback, thank you.
Looking carefully at the operation of LazyInitializer, and the new code,
they are very similar, and I can see an argument that the new code is not a
significant change. Let me list key differences, summarize the benefits of
the new code, and then open the question of next steps.
- LazyInitializer holds a JVM lock over the initialize() method call
- The new code does not hold a lock over the initialize() method call
- It is possible to create a JVM deadlock with LazyInitializer (I can
provide an example)
- It is possible to create the same deadlock with the new code, but not
as a JVM deadlock
- it's possible to break the deadlock without restarting the JVM
- the deadlock is created from having two threads both waiting
indefinitely for the other to notify it
- On exception from the initialize() method
- LazyInitializer will result in additional calls to the
initialize() method
(i.e. the guarantee for LazyInitializer is at most one *successful*
call to initialize())
- The new code will refuse to call initialize() again, instead
throwing a ConcurrentException with the matching message and cause as
the ConcurrentException resulting from the original call.
- The new code implements an interruptible wait; it allows threads, that
are waiting for another thread to finish calling initialize(), to abort
when interrupted.
Focusing only on the new code, here is how it operates:
- Guarantees initialize() is called at most one time as long as it
returns a non-null value (null return is a violation of the contract - i.e.
the results are non-deterministic)
- Guarantees initialize() is called with no locks held by the initializer
- Guarantees that all callers receive the same result
- The initialized object in the success case
- A ConcurrentException wrapping the root-cause exception thrown by
the initialize() method
- Allows subsequent threads waiting on the initialize() to complete to
be interrupted to abort the wait.
At the heart of the locking issue is one's comfort with JVM locks versus an
interruptible blocking wait.
So there is possible value here, but I see the argument that this code is
very similar in operation to LazyInitializer. While avoiding a JVM
deadlock can be useful, the operation of both sets of code is very similar
here. Can an application recover from a deadlock with the new code? Yes.
Will an application do so? That's unclear. Is it advisable to rely on the
ability to interrupt out of the deadlock? No. In that case, the root
cause of a deadlock needs to be eliminated. In that regard, the JVM
deadlock is a little simpler because jstack (and other tools) will clearly
identify and notify the cause of the deadlock. Could there be other
reasons to want to interrupt the wait? Yes.
At this point, I'm really starting to argue the nuances and true
value-to-cost ratio of a concurrent class of this nature. I personally am
very comfortable with the JVM concurrency primitives, and am aware that
tools that attempt to simplify the problem for developers can mislead them
into a false sense of security (e.g. LazyInitializer can easily be involved
in a deadlock).
Let me know your thoughts. This new implementation can be a valuable
addition, but this is not a simple matter for users.
Art
P.S. Sorry for the slow response.
On Thu, Feb 26, 2015 at 1:29 PM, Oliver Heger <ol...@oliver-heger.de>
wrote:
> Hi Arthur,
>
> I don't have any principle objections against your implementation.
>
> I just don't want it as a replacement for the AtomicSafeInitializer
> class. The latter was designed and announces itself to use only atomic
> variables for its synchronization and no other types of locks. This
> premise no longer holds. If the implementation is broken and does not
> work as expected, we should deprecate and later remove it.
>
> What I would be interested in is a comparison between your
> implementation and LazyInitializer. How do these classes behave in
> typical scenarios and which one is the preferred option under which
> conditions?
>
> If we add your proposal as an additional initializer implementation, we
> will also have to update the user's guide and give our users appropriate
> recommendations when they should use which class.
>
> And, we will have to pick a meaningful name for your class.
>
> Oliver
>
> Am 26.02.2015 um 20:51 schrieb Arthur Naseef:
> > A few questions coming to mind here:
> >
> > - First off, how do we move this forward?
> > - What do we do with the new implementation?
> > - What is a good way to address the busy-wait in the original code?
> > - What is a good way to address the fact that all threads will
> > busy-wait in the original code if the initialize() method throws an
> > exception?
> > - What is a good way to address the original comments which
> indicate
> > this class will perform better when, in fact, the busy waiting may
> have a
> > significant impact on performance?
> > - How can we address the fears? Honestly, I think the comment is
> less a
> > "fear" than a belief, so how do we prove/disprove?
> > - What is this other initializer implementation?
> > - What are the differences in operation between the two?
> >
> > Oliver - I'm reading a lot of concerns here, but not seeing how you would
> > like to address those concerns. Please help me on that front.
> >
> > Note, by the way, that the new implementation operates the same as the
> > original code, to the application using it, except in the case of an
> > exception thrown on the initialize() call, which is problematic now.
> That
> > is, this new implementation guarantees initialize() will only ever be
> > called one time, and it ensures all callers receive the result of that
> > initialize() method.
> >
> > Art
> >
> >
> >
> > On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <
> oliver.heger@oliver-heger.de>
> > wrote:
> >
> >>
> >>
> >> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
> >>> Oliver Heger has raised concerns about this commit in JIRA [1]:
> >>>
> >>>> This is a strong change in the behavior of this class. The main
> property
> >>> of atomic initializers was that they are non
> >>>> blocking. Now a blocking wait has been introduced. When there is so
> much
> >>> contention that the busy wait is
> >>>> actually a problem, wouln't it then be better to directly use a
> blocking
> >>> variant like lazy initializer?
> >>>
> >>> I've looked through the JavaDoc of AtomicInitializer once more. It
> says:
> >>> "Because {@code AtomicSafeInitializer} does not use synchronization at
> >> all
> >>> it probably outruns {@link LazyInitializer}, at least under low or
> >> moderate
> >>> concurrent access."
> >>>
> >>> This is the only thing I can find regarding concurrency properties of
> >>> AtomicInitializer. I think this still holds, doesn't it?
> >>
> >> No, the CountDownLatch is synchronized.
> >>
> >> There are multiple initializer implementations with different properties
> >> which are suitable for different use cases and scenarios. The atomic
> >> initializers are useful if you want to avoid blocking calls, but they
> >> might be an inferior solution under high contention.
> >>
> >> My fear is that this commit optimizes the class for a use case which can
> >> be served better by another initializer implementation which is already
> >> blocking; and this optimization has negative impact on the original
> >> intention of AtomicSafeInitializer.
> >>
> >> Oliver
> >>
> >>>
> >>> Benedikt
> >>>
> >>> [1] https://issues.apache.org/jira/browse/LANG-1086
> >>>
> >>> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
> >>>
> >>>> Author: britter
> >>>> Date: Mon Feb 23 20:15:49 2015
> >>>> New Revision: 1661762
> >>>>
> >>>> URL: http://svn.apache.org/r1661762
> >>>> Log:
> >>>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This
> also
> >>>> fixes #46 from github. Thanks to github user artnaseef.
> >>>>
> >>>> Modified:
> >>>> commons/proper/lang/trunk/src/changes/changes.xml
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>>
> >>>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8]
> (original)
> >>>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb
> 23
> >>>> 20:15:49 2015
> >>>> @@ -22,6 +22,7 @@
> >>>> <body>
> >>>>
> >>>> <release version="3.4" date="tba" description="tba">
> >>>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
> >>>> wait from AtomicSafeInitializer.get()</action>
> >>>> <action issue="LANG-1081" type="fix" dev="britter"
> due-to="Jonathan
> >>>> Baker">DiffBuilder.append(String, Object left, Object right) does not
> >> do a
> >>>> left.equals(right) check</action>
> >>>> <action issue="LANG-1055" type="fix" dev="britter"
> due-to="Jonathan
> >>>> Baker">StrSubstitutor.replaceSystemProperties does not work
> >>>> consistently</action>
> >>>> <action issue="LANG-1082" type="add" dev="britter"
> due-to="Jonathan
> >>>> Baker">Add option to disable the "objectsTriviallyEqual" test in
> >>>> DiffBuilder</action>
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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 AtomicSafeInitiali
> >>>> /** Holds the reference to the managed object. */
> >>>> private final AtomicReference<T> reference = new
> >> AtomicReference<T>();
> >>>>
> >>>> + /** Holds the exception that terminated the initialize() method,
> if
> >>>> an exception was thrown */
> >>>> + private final AtomicReference<ConcurrentException> referenceExc =
> >> new
> >>>> AtomicReference<ConcurrentException>();
> >>>> +
> >>>> + /** 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);
> >>>> + }
> >>>> }
> >>>> }
> >>>>
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
> >>>>
> >>>> 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 AbstractConcurrent
> >>>> @Test
> >>>> public void testGetConcurrent() throws ConcurrentException,
> >>>> InterruptedException {
> >>>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
> >>>> }
> >>>>
> >>>> // 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 AbstractConcurrent
> >>>> * @return the initializer object to be tested
> >>>> */
> >>>> protected abstract ConcurrentInitializer<Object>
> >> createInitializer();
> >>>> +
> >>>> + /**
> >>>> + * Creates a {@link ConcurrentInitializer} object that always
> >> throws
> >>>> + * exceptions.
> >>>> + *
> >>>> + * @return
> >>>> + */
> >>>> + protected abstract ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer();
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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 exten
> >>>> }
> >>>> };
> >>>> }
> >>>> +
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer() {
> >>>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> >> extends
> >>>> AtomicSafeInitializer<Object> {
> >>>> + @Override
> >>>> + protected Object initialize() throws ConcurrentException {
> >>>> + throw new ConcurrentException(testExceptionMessage,
> >>>> testCauseException);
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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);
> >>>> }
> >>>>
> >>>> /**
> >>>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
> >>>> }
> >>>>
> >>>> /**
> >>>> + * Returns the exception-throwing initializer to be tested.
> >>>> + *
> >>>> + * @return the {@code AtomicSafeInitializer} under test when
> >>>> validating
> >>>> + * exception handling
> >>>> + */
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer() {
> >>>> + return exceptionThrowingInitializer;
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> * Tests that initialize() is called only once.
> >>>> *
> >>>> * @throws
> org.apache.commons.lang3.concurrent.ConcurrentException
> >>>> because {@link #testGetConcurrent()} may throw it
> >>>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
> >>>> 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 e
> >>>> return new Object();
> >>>> }
> >>>> }
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> >> extends
> >>>> AtomicSafeInitializer<Object> {
> >>>> + @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<Object> {
> >>>> + 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<Object> initializer;
> >>>> + private Object result;
> >>>> + private boolean caughtException;
> >>>> +
> >>>> + public InitializerTestThread(AtomicSafeInitializer<Object>
> >>>> 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;
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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
> >>>> return initializer;
> >>>> }
> >>>>
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> 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
> >>>> return new Object();
> >>>> }
> >>>> }
> >>>> +
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingLazyInitializerTestImpl extends
> >>>> LazyInitializer<Object> {
> >>>> + @Override
> >>>> + protected Object initialize() throws ConcurrentException {
> >>>> + throw new ConcurrentException(testExceptionMessage,
> >>>> testCauseException);
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src: changes/
main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Oliver Heger <ol...@oliver-heger.de>.
Am 01.03.2015 um 17:52 schrieb Benedikt Ritter:
> 2015-02-26 21:29 GMT+01:00 Oliver Heger <ol...@oliver-heger.de>:
>
>> Hi Arthur,
>>
>> I don't have any principle objections against your implementation.
>>
>> I just don't want it as a replacement for the AtomicSafeInitializer
>> class. The latter was designed and announces itself to use only atomic
>> variables for its synchronization and no other types of locks. This
>> premise no longer holds. If the implementation is broken and does not
>> work as expected, we should deprecate and later remove it.
>>
>
> Just deprecating the class if it has a bug (hanging threads when an
> exception is thrown) doesn't seem right to me.
>
> How about we apply the patch and make it very clear in the release notes
> and JavaDoc, that it's behavior has been changed?
But then the name of the class is completely wrong. It is a bit like
public class ClassWithoutSynchronization {
public void doSomething() {
*synchronized*(this) {
...
}
}
}
For me the name AtomicSafeInitializer indicates that only atomic
variables are used to ensure thread-safety. Everything else would be
surprising, even if it was mentioned in JavaDoc.
Oliver
>
> Benedikt
>
>
>>
>> What I would be interested in is a comparison between your
>> implementation and LazyInitializer. How do these classes behave in
>> typical scenarios and which one is the preferred option under which
>> conditions?
>>
>> If we add your proposal as an additional initializer implementation, we
>> will also have to update the user's guide and give our users appropriate
>> recommendations when they should use which class.
>>
>> And, we will have to pick a meaningful name for your class.
>>
>> Oliver
>>
>> Am 26.02.2015 um 20:51 schrieb Arthur Naseef:
>>> A few questions coming to mind here:
>>>
>>> - First off, how do we move this forward?
>>> - What do we do with the new implementation?
>>> - What is a good way to address the busy-wait in the original code?
>>> - What is a good way to address the fact that all threads will
>>> busy-wait in the original code if the initialize() method throws an
>>> exception?
>>> - What is a good way to address the original comments which
>> indicate
>>> this class will perform better when, in fact, the busy waiting may
>> have a
>>> significant impact on performance?
>>> - How can we address the fears? Honestly, I think the comment is
>> less a
>>> "fear" than a belief, so how do we prove/disprove?
>>> - What is this other initializer implementation?
>>> - What are the differences in operation between the two?
>>>
>>> Oliver - I'm reading a lot of concerns here, but not seeing how you would
>>> like to address those concerns. Please help me on that front.
>>>
>>> Note, by the way, that the new implementation operates the same as the
>>> original code, to the application using it, except in the case of an
>>> exception thrown on the initialize() call, which is problematic now.
>> That
>>> is, this new implementation guarantees initialize() will only ever be
>>> called one time, and it ensures all callers receive the result of that
>>> initialize() method.
>>>
>>> Art
>>>
>>>
>>>
>>> On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <
>> oliver.heger@oliver-heger.de>
>>> wrote:
>>>
>>>>
>>>>
>>>> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
>>>>> Oliver Heger has raised concerns about this commit in JIRA [1]:
>>>>>
>>>>>> This is a strong change in the behavior of this class. The main
>> property
>>>>> of atomic initializers was that they are non
>>>>>> blocking. Now a blocking wait has been introduced. When there is so
>> much
>>>>> contention that the busy wait is
>>>>>> actually a problem, wouln't it then be better to directly use a
>> blocking
>>>>> variant like lazy initializer?
>>>>>
>>>>> I've looked through the JavaDoc of AtomicInitializer once more. It
>> says:
>>>>> "Because {@code AtomicSafeInitializer} does not use synchronization at
>>>> all
>>>>> it probably outruns {@link LazyInitializer}, at least under low or
>>>> moderate
>>>>> concurrent access."
>>>>>
>>>>> This is the only thing I can find regarding concurrency properties of
>>>>> AtomicInitializer. I think this still holds, doesn't it?
>>>>
>>>> No, the CountDownLatch is synchronized.
>>>>
>>>> There are multiple initializer implementations with different properties
>>>> which are suitable for different use cases and scenarios. The atomic
>>>> initializers are useful if you want to avoid blocking calls, but they
>>>> might be an inferior solution under high contention.
>>>>
>>>> My fear is that this commit optimizes the class for a use case which can
>>>> be served better by another initializer implementation which is already
>>>> blocking; and this optimization has negative impact on the original
>>>> intention of AtomicSafeInitializer.
>>>>
>>>> Oliver
>>>>
>>>>>
>>>>> Benedikt
>>>>>
>>>>> [1] https://issues.apache.org/jira/browse/LANG-1086
>>>>>
>>>>> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
>>>>>
>>>>>> Author: britter
>>>>>> Date: Mon Feb 23 20:15:49 2015
>>>>>> New Revision: 1661762
>>>>>>
>>>>>> URL: http://svn.apache.org/r1661762
>>>>>> Log:
>>>>>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This
>> also
>>>>>> fixes #46 from github. Thanks to github user artnaseef.
>>>>>>
>>>>>> Modified:
>>>>>> commons/proper/lang/trunk/src/changes/changes.xml
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>>
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>>
>>>>>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8]
>> (original)
>>>>>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb
>> 23
>>>>>> 20:15:49 2015
>>>>>> @@ -22,6 +22,7 @@
>>>>>> <body>
>>>>>>
>>>>>> <release version="3.4" date="tba" description="tba">
>>>>>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
>>>>>> wait from AtomicSafeInitializer.get()</action>
>>>>>> <action issue="LANG-1081" type="fix" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">DiffBuilder.append(String, Object left, Object right) does not
>>>> do a
>>>>>> left.equals(right) check</action>
>>>>>> <action issue="LANG-1055" type="fix" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">StrSubstitutor.replaceSystemProperties does not work
>>>>>> consistently</action>
>>>>>> <action issue="LANG-1082" type="add" dev="britter"
>> due-to="Jonathan
>>>>>> Baker">Add option to disable the "objectsTriviallyEqual" test in
>>>>>> DiffBuilder</action>
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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 AtomicSafeInitiali
>>>>>> /** Holds the reference to the managed object. */
>>>>>> private final AtomicReference<T> reference = new
>>>> AtomicReference<T>();
>>>>>>
>>>>>> + /** Holds the exception that terminated the initialize() method,
>> if
>>>>>> an exception was thrown */
>>>>>> + private final AtomicReference<ConcurrentException> referenceExc =
>>>> new
>>>>>> AtomicReference<ConcurrentException>();
>>>>>> +
>>>>>> + /** 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);
>>>>>> + }
>>>>>> }
>>>>>> }
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>>>>>>
>>>>>> 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 AbstractConcurrent
>>>>>> @Test
>>>>>> public void testGetConcurrent() throws ConcurrentException,
>>>>>> InterruptedException {
>>>>>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
>>>>>> }
>>>>>>
>>>>>> // 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 AbstractConcurrent
>>>>>> * @return the initializer object to be tested
>>>>>> */
>>>>>> protected abstract ConcurrentInitializer<Object>
>>>> createInitializer();
>>>>>> +
>>>>>> + /**
>>>>>> + * Creates a {@link ConcurrentInitializer} object that always
>>>> throws
>>>>>> + * exceptions.
>>>>>> + *
>>>>>> + * @return
>>>>>> + */
>>>>>> + protected abstract ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer();
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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 exten
>>>>>> }
>>>>>> };
>>>>>> }
>>>>>> +
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer() {
>>>>>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
>>>>>> + }
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>>>> extends
>>>>>> AtomicSafeInitializer<Object> {
>>>>>> + @Override
>>>>>> + protected Object initialize() throws ConcurrentException {
>>>>>> + throw new ConcurrentException(testExceptionMessage,
>>>>>> testCauseException);
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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);
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
>>>>>> }
>>>>>>
>>>>>> /**
>>>>>> + * Returns the exception-throwing initializer to be tested.
>>>>>> + *
>>>>>> + * @return the {@code AtomicSafeInitializer} under test when
>>>>>> validating
>>>>>> + * exception handling
>>>>>> + */
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> createExceptionThrowingInitializer() {
>>>>>> + return exceptionThrowingInitializer;
>>>>>> + }
>>>>>> +
>>>>>> + /**
>>>>>> * Tests that initialize() is called only once.
>>>>>> *
>>>>>> * @throws
>> org.apache.commons.lang3.concurrent.ConcurrentException
>>>>>> because {@link #testGetConcurrent()} may throw it
>>>>>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
>>>>>> 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 e
>>>>>> return new Object();
>>>>>> }
>>>>>> }
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>>>> extends
>>>>>> AtomicSafeInitializer<Object> {
>>>>>> + @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<Object> {
>>>>>> + 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<Object> initializer;
>>>>>> + private Object result;
>>>>>> + private boolean caughtException;
>>>>>> +
>>>>>> + public InitializerTestThread(AtomicSafeInitializer<Object>
>>>>>> 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;
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>> Modified:
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> URL:
>>>>>>
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>>>
>>>>>>
>>>>
>> ==============================================================================
>>>>>> ---
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> (original)
>>>>>> +++
>>>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>>> Mon Feb 23 20:15:49 2015
>>>>>> @@ -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
>>>>>> return initializer;
>>>>>> }
>>>>>>
>>>>>> + @Override
>>>>>> + protected ConcurrentInitializer<Object>
>>>>>> 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
>>>>>> return new Object();
>>>>>> }
>>>>>> }
>>>>>> +
>>>>>> +
>>>>>> + /**
>>>>>> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>>>>>> This
>>>>>> + * implementation always throws an exception.
>>>>>> + */
>>>>>> + private class ExceptionThrowingLazyInitializerTestImpl extends
>>>>>> LazyInitializer<Object> {
>>>>>> + @Override
>>>>>> + protected Object initialize() throws ConcurrentException {
>>>>>> + throw new ConcurrentException(testExceptionMessage,
>>>>>> testCauseException);
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Benedikt Ritter <br...@apache.org>.
2015-02-26 21:29 GMT+01:00 Oliver Heger <ol...@oliver-heger.de>:
> Hi Arthur,
>
> I don't have any principle objections against your implementation.
>
> I just don't want it as a replacement for the AtomicSafeInitializer
> class. The latter was designed and announces itself to use only atomic
> variables for its synchronization and no other types of locks. This
> premise no longer holds. If the implementation is broken and does not
> work as expected, we should deprecate and later remove it.
>
Just deprecating the class if it has a bug (hanging threads when an
exception is thrown) doesn't seem right to me.
How about we apply the patch and make it very clear in the release notes
and JavaDoc, that it's behavior has been changed?
Benedikt
>
> What I would be interested in is a comparison between your
> implementation and LazyInitializer. How do these classes behave in
> typical scenarios and which one is the preferred option under which
> conditions?
>
> If we add your proposal as an additional initializer implementation, we
> will also have to update the user's guide and give our users appropriate
> recommendations when they should use which class.
>
> And, we will have to pick a meaningful name for your class.
>
> Oliver
>
> Am 26.02.2015 um 20:51 schrieb Arthur Naseef:
> > A few questions coming to mind here:
> >
> > - First off, how do we move this forward?
> > - What do we do with the new implementation?
> > - What is a good way to address the busy-wait in the original code?
> > - What is a good way to address the fact that all threads will
> > busy-wait in the original code if the initialize() method throws an
> > exception?
> > - What is a good way to address the original comments which
> indicate
> > this class will perform better when, in fact, the busy waiting may
> have a
> > significant impact on performance?
> > - How can we address the fears? Honestly, I think the comment is
> less a
> > "fear" than a belief, so how do we prove/disprove?
> > - What is this other initializer implementation?
> > - What are the differences in operation between the two?
> >
> > Oliver - I'm reading a lot of concerns here, but not seeing how you would
> > like to address those concerns. Please help me on that front.
> >
> > Note, by the way, that the new implementation operates the same as the
> > original code, to the application using it, except in the case of an
> > exception thrown on the initialize() call, which is problematic now.
> That
> > is, this new implementation guarantees initialize() will only ever be
> > called one time, and it ensures all callers receive the result of that
> > initialize() method.
> >
> > Art
> >
> >
> >
> > On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <
> oliver.heger@oliver-heger.de>
> > wrote:
> >
> >>
> >>
> >> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
> >>> Oliver Heger has raised concerns about this commit in JIRA [1]:
> >>>
> >>>> This is a strong change in the behavior of this class. The main
> property
> >>> of atomic initializers was that they are non
> >>>> blocking. Now a blocking wait has been introduced. When there is so
> much
> >>> contention that the busy wait is
> >>>> actually a problem, wouln't it then be better to directly use a
> blocking
> >>> variant like lazy initializer?
> >>>
> >>> I've looked through the JavaDoc of AtomicInitializer once more. It
> says:
> >>> "Because {@code AtomicSafeInitializer} does not use synchronization at
> >> all
> >>> it probably outruns {@link LazyInitializer}, at least under low or
> >> moderate
> >>> concurrent access."
> >>>
> >>> This is the only thing I can find regarding concurrency properties of
> >>> AtomicInitializer. I think this still holds, doesn't it?
> >>
> >> No, the CountDownLatch is synchronized.
> >>
> >> There are multiple initializer implementations with different properties
> >> which are suitable for different use cases and scenarios. The atomic
> >> initializers are useful if you want to avoid blocking calls, but they
> >> might be an inferior solution under high contention.
> >>
> >> My fear is that this commit optimizes the class for a use case which can
> >> be served better by another initializer implementation which is already
> >> blocking; and this optimization has negative impact on the original
> >> intention of AtomicSafeInitializer.
> >>
> >> Oliver
> >>
> >>>
> >>> Benedikt
> >>>
> >>> [1] https://issues.apache.org/jira/browse/LANG-1086
> >>>
> >>> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
> >>>
> >>>> Author: britter
> >>>> Date: Mon Feb 23 20:15:49 2015
> >>>> New Revision: 1661762
> >>>>
> >>>> URL: http://svn.apache.org/r1661762
> >>>> Log:
> >>>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This
> also
> >>>> fixes #46 from github. Thanks to github user artnaseef.
> >>>>
> >>>> Modified:
> >>>> commons/proper/lang/trunk/src/changes/changes.xml
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>>
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>>
> >>>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8]
> (original)
> >>>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb
> 23
> >>>> 20:15:49 2015
> >>>> @@ -22,6 +22,7 @@
> >>>> <body>
> >>>>
> >>>> <release version="3.4" date="tba" description="tba">
> >>>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
> >>>> wait from AtomicSafeInitializer.get()</action>
> >>>> <action issue="LANG-1081" type="fix" dev="britter"
> due-to="Jonathan
> >>>> Baker">DiffBuilder.append(String, Object left, Object right) does not
> >> do a
> >>>> left.equals(right) check</action>
> >>>> <action issue="LANG-1055" type="fix" dev="britter"
> due-to="Jonathan
> >>>> Baker">StrSubstitutor.replaceSystemProperties does not work
> >>>> consistently</action>
> >>>> <action issue="LANG-1082" type="add" dev="britter"
> due-to="Jonathan
> >>>> Baker">Add option to disable the "objectsTriviallyEqual" test in
> >>>> DiffBuilder</action>
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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 AtomicSafeInitiali
> >>>> /** Holds the reference to the managed object. */
> >>>> private final AtomicReference<T> reference = new
> >> AtomicReference<T>();
> >>>>
> >>>> + /** Holds the exception that terminated the initialize() method,
> if
> >>>> an exception was thrown */
> >>>> + private final AtomicReference<ConcurrentException> referenceExc =
> >> new
> >>>> AtomicReference<ConcurrentException>();
> >>>> +
> >>>> + /** 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);
> >>>> + }
> >>>> }
> >>>> }
> >>>>
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
> >>>>
> >>>> 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 AbstractConcurrent
> >>>> @Test
> >>>> public void testGetConcurrent() throws ConcurrentException,
> >>>> InterruptedException {
> >>>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
> >>>> }
> >>>>
> >>>> // 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 AbstractConcurrent
> >>>> * @return the initializer object to be tested
> >>>> */
> >>>> protected abstract ConcurrentInitializer<Object>
> >> createInitializer();
> >>>> +
> >>>> + /**
> >>>> + * Creates a {@link ConcurrentInitializer} object that always
> >> throws
> >>>> + * exceptions.
> >>>> + *
> >>>> + * @return
> >>>> + */
> >>>> + protected abstract ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer();
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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 exten
> >>>> }
> >>>> };
> >>>> }
> >>>> +
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer() {
> >>>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> >> extends
> >>>> AtomicSafeInitializer<Object> {
> >>>> + @Override
> >>>> + protected Object initialize() throws ConcurrentException {
> >>>> + throw new ConcurrentException(testExceptionMessage,
> >>>> testCauseException);
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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);
> >>>> }
> >>>>
> >>>> /**
> >>>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
> >>>> }
> >>>>
> >>>> /**
> >>>> + * Returns the exception-throwing initializer to be tested.
> >>>> + *
> >>>> + * @return the {@code AtomicSafeInitializer} under test when
> >>>> validating
> >>>> + * exception handling
> >>>> + */
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> createExceptionThrowingInitializer() {
> >>>> + return exceptionThrowingInitializer;
> >>>> + }
> >>>> +
> >>>> + /**
> >>>> * Tests that initialize() is called only once.
> >>>> *
> >>>> * @throws
> org.apache.commons.lang3.concurrent.ConcurrentException
> >>>> because {@link #testGetConcurrent()} may throw it
> >>>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
> >>>> 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 e
> >>>> return new Object();
> >>>> }
> >>>> }
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> >> extends
> >>>> AtomicSafeInitializer<Object> {
> >>>> + @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<Object> {
> >>>> + 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<Object> initializer;
> >>>> + private Object result;
> >>>> + private boolean caughtException;
> >>>> +
> >>>> + public InitializerTestThread(AtomicSafeInitializer<Object>
> >>>> 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;
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>> Modified:
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> URL:
> >>>>
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>>>
> >>>>
> >>
> ==============================================================================
> >>>> ---
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> (original)
> >>>> +++
> >>>>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>>> Mon Feb 23 20:15:49 2015
> >>>> @@ -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
> >>>> return initializer;
> >>>> }
> >>>>
> >>>> + @Override
> >>>> + protected ConcurrentInitializer<Object>
> >>>> 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
> >>>> return new Object();
> >>>> }
> >>>> }
> >>>> +
> >>>> +
> >>>> + /**
> >>>> + * A concrete test implementation of {@code
> AtomicSafeInitializer}.
> >>>> This
> >>>> + * implementation always throws an exception.
> >>>> + */
> >>>> + private class ExceptionThrowingLazyInitializerTestImpl extends
> >>>> LazyInitializer<Object> {
> >>>> + @Override
> >>>> + protected Object initialize() throws ConcurrentException {
> >>>> + throw new ConcurrentException(testExceptionMessage,
> >>>> testCauseException);
> >>>> + }
> >>>> + }
> >>>> }
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> >> For additional commands, e-mail: dev-help@commons.apache.org
> >>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src: changes/
main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Oliver Heger <ol...@oliver-heger.de>.
Hi Arthur,
I don't have any principle objections against your implementation.
I just don't want it as a replacement for the AtomicSafeInitializer
class. The latter was designed and announces itself to use only atomic
variables for its synchronization and no other types of locks. This
premise no longer holds. If the implementation is broken and does not
work as expected, we should deprecate and later remove it.
What I would be interested in is a comparison between your
implementation and LazyInitializer. How do these classes behave in
typical scenarios and which one is the preferred option under which
conditions?
If we add your proposal as an additional initializer implementation, we
will also have to update the user's guide and give our users appropriate
recommendations when they should use which class.
And, we will have to pick a meaningful name for your class.
Oliver
Am 26.02.2015 um 20:51 schrieb Arthur Naseef:
> A few questions coming to mind here:
>
> - First off, how do we move this forward?
> - What do we do with the new implementation?
> - What is a good way to address the busy-wait in the original code?
> - What is a good way to address the fact that all threads will
> busy-wait in the original code if the initialize() method throws an
> exception?
> - What is a good way to address the original comments which indicate
> this class will perform better when, in fact, the busy waiting may have a
> significant impact on performance?
> - How can we address the fears? Honestly, I think the comment is less a
> "fear" than a belief, so how do we prove/disprove?
> - What is this other initializer implementation?
> - What are the differences in operation between the two?
>
> Oliver - I'm reading a lot of concerns here, but not seeing how you would
> like to address those concerns. Please help me on that front.
>
> Note, by the way, that the new implementation operates the same as the
> original code, to the application using it, except in the case of an
> exception thrown on the initialize() call, which is problematic now. That
> is, this new implementation guarantees initialize() will only ever be
> called one time, and it ensures all callers receive the result of that
> initialize() method.
>
> Art
>
>
>
> On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <ol...@oliver-heger.de>
> wrote:
>
>>
>>
>> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
>>> Oliver Heger has raised concerns about this commit in JIRA [1]:
>>>
>>>> This is a strong change in the behavior of this class. The main property
>>> of atomic initializers was that they are non
>>>> blocking. Now a blocking wait has been introduced. When there is so much
>>> contention that the busy wait is
>>>> actually a problem, wouln't it then be better to directly use a blocking
>>> variant like lazy initializer?
>>>
>>> I've looked through the JavaDoc of AtomicInitializer once more. It says:
>>> "Because {@code AtomicSafeInitializer} does not use synchronization at
>> all
>>> it probably outruns {@link LazyInitializer}, at least under low or
>> moderate
>>> concurrent access."
>>>
>>> This is the only thing I can find regarding concurrency properties of
>>> AtomicInitializer. I think this still holds, doesn't it?
>>
>> No, the CountDownLatch is synchronized.
>>
>> There are multiple initializer implementations with different properties
>> which are suitable for different use cases and scenarios. The atomic
>> initializers are useful if you want to avoid blocking calls, but they
>> might be an inferior solution under high contention.
>>
>> My fear is that this commit optimizes the class for a use case which can
>> be served better by another initializer implementation which is already
>> blocking; and this optimization has negative impact on the original
>> intention of AtomicSafeInitializer.
>>
>> Oliver
>>
>>>
>>> Benedikt
>>>
>>> [1] https://issues.apache.org/jira/browse/LANG-1086
>>>
>>> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
>>>
>>>> Author: britter
>>>> Date: Mon Feb 23 20:15:49 2015
>>>> New Revision: 1661762
>>>>
>>>> URL: http://svn.apache.org/r1661762
>>>> Log:
>>>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
>>>> fixes #46 from github. Thanks to github user artnaseef.
>>>>
>>>> Modified:
>>>> commons/proper/lang/trunk/src/changes/changes.xml
>>>>
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>>
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>>
>>>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
>>>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23
>>>> 20:15:49 2015
>>>> @@ -22,6 +22,7 @@
>>>> <body>
>>>>
>>>> <release version="3.4" date="tba" description="tba">
>>>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
>>>> wait from AtomicSafeInitializer.get()</action>
>>>> <action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan
>>>> Baker">DiffBuilder.append(String, Object left, Object right) does not
>> do a
>>>> left.equals(right) check</action>
>>>> <action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan
>>>> Baker">StrSubstitutor.replaceSystemProperties does not work
>>>> consistently</action>
>>>> <action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan
>>>> Baker">Add option to disable the "objectsTriviallyEqual" test in
>>>> DiffBuilder</action>
>>>>
>>>> Modified:
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> ---
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>> (original)
>>>> +++
>>>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>>> Mon Feb 23 20:15:49 2015
>>>> @@ -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 AtomicSafeInitiali
>>>> /** Holds the reference to the managed object. */
>>>> private final AtomicReference<T> reference = new
>> AtomicReference<T>();
>>>>
>>>> + /** Holds the exception that terminated the initialize() method, if
>>>> an exception was thrown */
>>>> + private final AtomicReference<ConcurrentException> referenceExc =
>> new
>>>> AtomicReference<ConcurrentException>();
>>>> +
>>>> + /** 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);
>>>> + }
>>>> }
>>>> }
>>>>
>>>>
>>>> Modified:
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> ---
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>> (original)
>>>> +++
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>>> Mon Feb 23 20:15:49 2015
>>>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>>>>
>>>> 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 AbstractConcurrent
>>>> @Test
>>>> public void testGetConcurrent() throws ConcurrentException,
>>>> InterruptedException {
>>>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
>>>> }
>>>>
>>>> // 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 AbstractConcurrent
>>>> * @return the initializer object to be tested
>>>> */
>>>> protected abstract ConcurrentInitializer<Object>
>> createInitializer();
>>>> +
>>>> + /**
>>>> + * Creates a {@link ConcurrentInitializer} object that always
>> throws
>>>> + * exceptions.
>>>> + *
>>>> + * @return
>>>> + */
>>>> + protected abstract ConcurrentInitializer<Object>
>>>> createExceptionThrowingInitializer();
>>>> }
>>>>
>>>> Modified:
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> ---
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>> (original)
>>>> +++
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>>> Mon Feb 23 20:15:49 2015
>>>> @@ -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 exten
>>>> }
>>>> };
>>>> }
>>>> +
>>>> + @Override
>>>> + protected ConcurrentInitializer<Object>
>>>> createExceptionThrowingInitializer() {
>>>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
>>>> + }
>>>> +
>>>> + /**
>>>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>>>> This
>>>> + * implementation always throws an exception.
>>>> + */
>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>> extends
>>>> AtomicSafeInitializer<Object> {
>>>> + @Override
>>>> + protected Object initialize() throws ConcurrentException {
>>>> + throw new ConcurrentException(testExceptionMessage,
>>>> testCauseException);
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> Modified:
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> ---
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>> (original)
>>>> +++
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>>> Mon Feb 23 20:15:49 2015
>>>> @@ -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);
>>>> }
>>>>
>>>> /**
>>>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
>>>> }
>>>>
>>>> /**
>>>> + * Returns the exception-throwing initializer to be tested.
>>>> + *
>>>> + * @return the {@code AtomicSafeInitializer} under test when
>>>> validating
>>>> + * exception handling
>>>> + */
>>>> + @Override
>>>> + protected ConcurrentInitializer<Object>
>>>> createExceptionThrowingInitializer() {
>>>> + return exceptionThrowingInitializer;
>>>> + }
>>>> +
>>>> + /**
>>>> * Tests that initialize() is called only once.
>>>> *
>>>> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
>>>> because {@link #testGetConcurrent()} may throw it
>>>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
>>>> 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 e
>>>> return new Object();
>>>> }
>>>> }
>>>> +
>>>> + /**
>>>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>>>> This
>>>> + * implementation always throws an exception.
>>>> + */
>>>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>> extends
>>>> AtomicSafeInitializer<Object> {
>>>> + @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<Object> {
>>>> + 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<Object> initializer;
>>>> + private Object result;
>>>> + private boolean caughtException;
>>>> +
>>>> + public InitializerTestThread(AtomicSafeInitializer<Object>
>>>> 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;
>>>> + }
>>>> + }
>>>> }
>>>>
>>>> Modified:
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>> URL:
>>>>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>>>
>>>>
>> ==============================================================================
>>>> ---
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>> (original)
>>>> +++
>>>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>>> Mon Feb 23 20:15:49 2015
>>>> @@ -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
>>>> return initializer;
>>>> }
>>>>
>>>> + @Override
>>>> + protected ConcurrentInitializer<Object>
>>>> 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
>>>> return new Object();
>>>> }
>>>> }
>>>> +
>>>> +
>>>> + /**
>>>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>>>> This
>>>> + * implementation always throws an exception.
>>>> + */
>>>> + private class ExceptionThrowingLazyInitializerTestImpl extends
>>>> LazyInitializer<Object> {
>>>> + @Override
>>>> + protected Object initialize() throws ConcurrentException {
>>>> + throw new ConcurrentException(testExceptionMessage,
>>>> testCauseException);
>>>> + }
>>>> + }
>>>> }
>>>>
>>>>
>>>>
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Arthur Naseef <ar...@amlinv.com>.
A few questions coming to mind here:
- First off, how do we move this forward?
- What do we do with the new implementation?
- What is a good way to address the busy-wait in the original code?
- What is a good way to address the fact that all threads will
busy-wait in the original code if the initialize() method throws an
exception?
- What is a good way to address the original comments which indicate
this class will perform better when, in fact, the busy waiting may have a
significant impact on performance?
- How can we address the fears? Honestly, I think the comment is less a
"fear" than a belief, so how do we prove/disprove?
- What is this other initializer implementation?
- What are the differences in operation between the two?
Oliver - I'm reading a lot of concerns here, but not seeing how you would
like to address those concerns. Please help me on that front.
Note, by the way, that the new implementation operates the same as the
original code, to the application using it, except in the case of an
exception thrown on the initialize() call, which is problematic now. That
is, this new implementation guarantees initialize() will only ever be
called one time, and it ensures all callers receive the result of that
initialize() method.
Art
On Mon, Feb 23, 2015 at 1:47 PM, Oliver Heger <ol...@oliver-heger.de>
wrote:
>
>
> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
> > Oliver Heger has raised concerns about this commit in JIRA [1]:
> >
> >> This is a strong change in the behavior of this class. The main property
> > of atomic initializers was that they are non
> >> blocking. Now a blocking wait has been introduced. When there is so much
> > contention that the busy wait is
> >> actually a problem, wouln't it then be better to directly use a blocking
> > variant like lazy initializer?
> >
> > I've looked through the JavaDoc of AtomicInitializer once more. It says:
> > "Because {@code AtomicSafeInitializer} does not use synchronization at
> all
> > it probably outruns {@link LazyInitializer}, at least under low or
> moderate
> > concurrent access."
> >
> > This is the only thing I can find regarding concurrency properties of
> > AtomicInitializer. I think this still holds, doesn't it?
>
> No, the CountDownLatch is synchronized.
>
> There are multiple initializer implementations with different properties
> which are suitable for different use cases and scenarios. The atomic
> initializers are useful if you want to avoid blocking calls, but they
> might be an inferior solution under high contention.
>
> My fear is that this commit optimizes the class for a use case which can
> be served better by another initializer implementation which is already
> blocking; and this optimization has negative impact on the original
> intention of AtomicSafeInitializer.
>
> Oliver
>
> >
> > Benedikt
> >
> > [1] https://issues.apache.org/jira/browse/LANG-1086
> >
> > 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
> >
> >> Author: britter
> >> Date: Mon Feb 23 20:15:49 2015
> >> New Revision: 1661762
> >>
> >> URL: http://svn.apache.org/r1661762
> >> Log:
> >> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
> >> fixes #46 from github. Thanks to github user artnaseef.
> >>
> >> Modified:
> >> commons/proper/lang/trunk/src/changes/changes.xml
> >>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>
> >> Modified: commons/proper/lang/trunk/src/changes/changes.xml
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
> >> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23
> >> 20:15:49 2015
> >> @@ -22,6 +22,7 @@
> >> <body>
> >>
> >> <release version="3.4" date="tba" description="tba">
> >> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
> >> wait from AtomicSafeInitializer.get()</action>
> >> <action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan
> >> Baker">DiffBuilder.append(String, Object left, Object right) does not
> do a
> >> left.equals(right) check</action>
> >> <action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan
> >> Baker">StrSubstitutor.replaceSystemProperties does not work
> >> consistently</action>
> >> <action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan
> >> Baker">Add option to disable the "objectsTriviallyEqual" test in
> >> DiffBuilder</action>
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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 AtomicSafeInitiali
> >> /** Holds the reference to the managed object. */
> >> private final AtomicReference<T> reference = new
> AtomicReference<T>();
> >>
> >> + /** Holds the exception that terminated the initialize() method, if
> >> an exception was thrown */
> >> + private final AtomicReference<ConcurrentException> referenceExc =
> new
> >> AtomicReference<ConcurrentException>();
> >> +
> >> + /** 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);
> >> + }
> >> }
> >> }
> >>
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
> >>
> >> 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 AbstractConcurrent
> >> @Test
> >> public void testGetConcurrent() throws ConcurrentException,
> >> InterruptedException {
> >> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
> >> }
> >>
> >> // 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 AbstractConcurrent
> >> * @return the initializer object to be tested
> >> */
> >> protected abstract ConcurrentInitializer<Object>
> createInitializer();
> >> +
> >> + /**
> >> + * Creates a {@link ConcurrentInitializer} object that always
> throws
> >> + * exceptions.
> >> + *
> >> + * @return
> >> + */
> >> + protected abstract ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer();
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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 exten
> >> }
> >> };
> >> }
> >> +
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer() {
> >> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
> >> + }
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> extends
> >> AtomicSafeInitializer<Object> {
> >> + @Override
> >> + protected Object initialize() throws ConcurrentException {
> >> + throw new ConcurrentException(testExceptionMessage,
> >> testCauseException);
> >> + }
> >> + }
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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);
> >> }
> >>
> >> /**
> >> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
> >> }
> >>
> >> /**
> >> + * Returns the exception-throwing initializer to be tested.
> >> + *
> >> + * @return the {@code AtomicSafeInitializer} under test when
> >> validating
> >> + * exception handling
> >> + */
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer() {
> >> + return exceptionThrowingInitializer;
> >> + }
> >> +
> >> + /**
> >> * Tests that initialize() is called only once.
> >> *
> >> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
> >> because {@link #testGetConcurrent()} may throw it
> >> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
> >> 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 e
> >> return new Object();
> >> }
> >> }
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> extends
> >> AtomicSafeInitializer<Object> {
> >> + @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<Object> {
> >> + 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<Object> initializer;
> >> + private Object result;
> >> + private boolean caughtException;
> >> +
> >> + public InitializerTestThread(AtomicSafeInitializer<Object>
> >> 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;
> >> + }
> >> + }
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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
> >> return initializer;
> >> }
> >>
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> 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
> >> return new Object();
> >> }
> >> }
> >> +
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingLazyInitializerTestImpl extends
> >> LazyInitializer<Object> {
> >> + @Override
> >> + protected Object initialize() throws ConcurrentException {
> >> + throw new ConcurrentException(testExceptionMessage,
> >> testCauseException);
> >> + }
> >> + }
> >> }
> >>
> >>
> >>
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Benedikt Ritter <br...@apache.org>.
2015-02-25 15:20 GMT+01:00 Benedikt Ritter <br...@apache.org>:
>
>
> 2015-02-23 21:47 GMT+01:00 Oliver Heger <ol...@oliver-heger.de>:
>
>>
>>
>> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
>> > Oliver Heger has raised concerns about this commit in JIRA [1]:
>> >
>> >> This is a strong change in the behavior of this class. The main
>> property
>> > of atomic initializers was that they are non
>> >> blocking. Now a blocking wait has been introduced. When there is so
>> much
>> > contention that the busy wait is
>> >> actually a problem, wouln't it then be better to directly use a
>> blocking
>> > variant like lazy initializer?
>> >
>> > I've looked through the JavaDoc of AtomicInitializer once more. It says:
>> > "Because {@code AtomicSafeInitializer} does not use synchronization at
>> all
>> > it probably outruns {@link LazyInitializer}, at least under low or
>> moderate
>> > concurrent access."
>> >
>> > This is the only thing I can find regarding concurrency properties of
>> > AtomicInitializer. I think this still holds, doesn't it?
>>
>> No, the CountDownLatch is synchronized.
>>
>> There are multiple initializer implementations with different properties
>> which are suitable for different use cases and scenarios. The atomic
>> initializers are useful if you want to avoid blocking calls, but they
>> might be an inferior solution under high contention.
>>
>> My fear is that this commit optimizes the class for a use case which can
>> be served better by another initializer implementation which is already
>> blocking; and this optimization has negative impact on the original
>> intention of AtomicSafeInitializer.
>>
>
> Okay. Looks like we should address this intend better in the JavaDoc of
> AtomicSafeInitializer. Maybe it should even have a better name like
> BusyWaitInitializer (could be changed for lang4).
>
> Do you think the changes made to AtomicSafeInitializer are an optimization
> that would better fit into LazyInitializer?
>
> I'll revert this change as soon as I get the time.
>
The change has been reverted in revision 1662379.
>
> Benedikt
>
>
>>
>> Oliver
>>
>> >
>> > Benedikt
>> >
>> > [1] https://issues.apache.org/jira/browse/LANG-1086
>> >
>> > 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
>> >
>> >> Author: britter
>> >> Date: Mon Feb 23 20:15:49 2015
>> >> New Revision: 1661762
>> >>
>> >> URL: http://svn.apache.org/r1661762
>> >> Log:
>> >> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
>> >> fixes #46 from github. Thanks to github user artnaseef.
>> >>
>> >> Modified:
>> >> commons/proper/lang/trunk/src/changes/changes.xml
>> >>
>> >>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> >>
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> >>
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> >>
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> >>
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> >>
>> >> Modified: commons/proper/lang/trunk/src/changes/changes.xml
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8]
>> (original)
>> >> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb
>> 23
>> >> 20:15:49 2015
>> >> @@ -22,6 +22,7 @@
>> >> <body>
>> >>
>> >> <release version="3.4" date="tba" description="tba">
>> >> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
>> >> wait from AtomicSafeInitializer.get()</action>
>> >> <action issue="LANG-1081" type="fix" dev="britter"
>> due-to="Jonathan
>> >> Baker">DiffBuilder.append(String, Object left, Object right) does not
>> do a
>> >> left.equals(right) check</action>
>> >> <action issue="LANG-1055" type="fix" dev="britter"
>> due-to="Jonathan
>> >> Baker">StrSubstitutor.replaceSystemProperties does not work
>> >> consistently</action>
>> >> <action issue="LANG-1082" type="add" dev="britter"
>> due-to="Jonathan
>> >> Baker">Add option to disable the "objectsTriviallyEqual" test in
>> >> DiffBuilder</action>
>> >>
>> >> Modified:
>> >>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> >> (original)
>> >> +++
>> >>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> >> Mon Feb 23 20:15:49 2015
>> >> @@ -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 AtomicSafeInitiali
>> >> /** Holds the reference to the managed object. */
>> >> private final AtomicReference<T> reference = new
>> AtomicReference<T>();
>> >>
>> >> + /** Holds the exception that terminated the initialize() method,
>> if
>> >> an exception was thrown */
>> >> + private final AtomicReference<ConcurrentException> referenceExc =
>> new
>> >> AtomicReference<ConcurrentException>();
>> >> +
>> >> + /** 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);
>> >> + }
>> >> }
>> >> }
>> >>
>> >>
>> >> Modified:
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> >> (original)
>> >> +++
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> >> Mon Feb 23 20:15:49 2015
>> >> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>> >>
>> >> 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 AbstractConcurrent
>> >> @Test
>> >> public void testGetConcurrent() throws ConcurrentException,
>> >> InterruptedException {
>> >> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
>> >> }
>> >>
>> >> // 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 AbstractConcurrent
>> >> * @return the initializer object to be tested
>> >> */
>> >> protected abstract ConcurrentInitializer<Object>
>> createInitializer();
>> >> +
>> >> + /**
>> >> + * Creates a {@link ConcurrentInitializer} object that always
>> throws
>> >> + * exceptions.
>> >> + *
>> >> + * @return
>> >> + */
>> >> + protected abstract ConcurrentInitializer<Object>
>> >> createExceptionThrowingInitializer();
>> >> }
>> >>
>> >> Modified:
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> >> (original)
>> >> +++
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> >> Mon Feb 23 20:15:49 2015
>> >> @@ -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 exten
>> >> }
>> >> };
>> >> }
>> >> +
>> >> + @Override
>> >> + protected ConcurrentInitializer<Object>
>> >> createExceptionThrowingInitializer() {
>> >> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
>> >> + }
>> >> +
>> >> + /**
>> >> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>> >> This
>> >> + * implementation always throws an exception.
>> >> + */
>> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>> extends
>> >> AtomicSafeInitializer<Object> {
>> >> + @Override
>> >> + protected Object initialize() throws ConcurrentException {
>> >> + throw new ConcurrentException(testExceptionMessage,
>> >> testCauseException);
>> >> + }
>> >> + }
>> >> }
>> >>
>> >> Modified:
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> >> (original)
>> >> +++
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> >> Mon Feb 23 20:15:49 2015
>> >> @@ -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);
>> >> }
>> >>
>> >> /**
>> >> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
>> >> }
>> >>
>> >> /**
>> >> + * Returns the exception-throwing initializer to be tested.
>> >> + *
>> >> + * @return the {@code AtomicSafeInitializer} under test when
>> >> validating
>> >> + * exception handling
>> >> + */
>> >> + @Override
>> >> + protected ConcurrentInitializer<Object>
>> >> createExceptionThrowingInitializer() {
>> >> + return exceptionThrowingInitializer;
>> >> + }
>> >> +
>> >> + /**
>> >> * Tests that initialize() is called only once.
>> >> *
>> >> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
>> >> because {@link #testGetConcurrent()} may throw it
>> >> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
>> >> 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 e
>> >> return new Object();
>> >> }
>> >> }
>> >> +
>> >> + /**
>> >> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>> >> This
>> >> + * implementation always throws an exception.
>> >> + */
>> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
>> extends
>> >> AtomicSafeInitializer<Object> {
>> >> + @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<Object> {
>> >> + 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<Object> initializer;
>> >> + private Object result;
>> >> + private boolean caughtException;
>> >> +
>> >> + public InitializerTestThread(AtomicSafeInitializer<Object>
>> >> 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;
>> >> + }
>> >> + }
>> >> }
>> >>
>> >> Modified:
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> >> URL:
>> >>
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>> >>
>> >>
>> ==============================================================================
>> >> ---
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> >> (original)
>> >> +++
>> >>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> >> Mon Feb 23 20:15:49 2015
>> >> @@ -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
>> >> return initializer;
>> >> }
>> >>
>> >> + @Override
>> >> + protected ConcurrentInitializer<Object>
>> >> 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
>> >> return new Object();
>> >> }
>> >> }
>> >> +
>> >> +
>> >> + /**
>> >> + * A concrete test implementation of {@code
>> AtomicSafeInitializer}.
>> >> This
>> >> + * implementation always throws an exception.
>> >> + */
>> >> + private class ExceptionThrowingLazyInitializerTestImpl extends
>> >> LazyInitializer<Object> {
>> >> + @Override
>> >> + protected Object initialize() throws ConcurrentException {
>> >> + throw new ConcurrentException(testExceptionMessage,
>> >> testCauseException);
>> >> + }
>> >> + }
>> >> }
>> >>
>> >>
>> >>
>> >
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Benedikt Ritter <br...@apache.org>.
2015-02-23 21:47 GMT+01:00 Oliver Heger <ol...@oliver-heger.de>:
>
>
> Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
> > Oliver Heger has raised concerns about this commit in JIRA [1]:
> >
> >> This is a strong change in the behavior of this class. The main property
> > of atomic initializers was that they are non
> >> blocking. Now a blocking wait has been introduced. When there is so much
> > contention that the busy wait is
> >> actually a problem, wouln't it then be better to directly use a blocking
> > variant like lazy initializer?
> >
> > I've looked through the JavaDoc of AtomicInitializer once more. It says:
> > "Because {@code AtomicSafeInitializer} does not use synchronization at
> all
> > it probably outruns {@link LazyInitializer}, at least under low or
> moderate
> > concurrent access."
> >
> > This is the only thing I can find regarding concurrency properties of
> > AtomicInitializer. I think this still holds, doesn't it?
>
> No, the CountDownLatch is synchronized.
>
> There are multiple initializer implementations with different properties
> which are suitable for different use cases and scenarios. The atomic
> initializers are useful if you want to avoid blocking calls, but they
> might be an inferior solution under high contention.
>
> My fear is that this commit optimizes the class for a use case which can
> be served better by another initializer implementation which is already
> blocking; and this optimization has negative impact on the original
> intention of AtomicSafeInitializer.
>
Okay. Looks like we should address this intend better in the JavaDoc of
AtomicSafeInitializer. Maybe it should even have a better name like
BusyWaitInitializer (could be changed for lang4).
Do you think the changes made to AtomicSafeInitializer are an optimization
that would better fit into LazyInitializer?
I'll revert this change as soon as I get the time.
Benedikt
>
> Oliver
>
> >
> > Benedikt
> >
> > [1] https://issues.apache.org/jira/browse/LANG-1086
> >
> > 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
> >
> >> Author: britter
> >> Date: Mon Feb 23 20:15:49 2015
> >> New Revision: 1661762
> >>
> >> URL: http://svn.apache.org/r1661762
> >> Log:
> >> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
> >> fixes #46 from github. Thanks to github user artnaseef.
> >>
> >> Modified:
> >> commons/proper/lang/trunk/src/changes/changes.xml
> >>
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >>
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >>
> >> Modified: commons/proper/lang/trunk/src/changes/changes.xml
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
> >> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23
> >> 20:15:49 2015
> >> @@ -22,6 +22,7 @@
> >> <body>
> >>
> >> <release version="3.4" date="tba" description="tba">
> >> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
> >> wait from AtomicSafeInitializer.get()</action>
> >> <action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan
> >> Baker">DiffBuilder.append(String, Object left, Object right) does not
> do a
> >> left.equals(right) check</action>
> >> <action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan
> >> Baker">StrSubstitutor.replaceSystemProperties does not work
> >> consistently</action>
> >> <action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan
> >> Baker">Add option to disable the "objectsTriviallyEqual" test in
> >> DiffBuilder</action>
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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 AtomicSafeInitiali
> >> /** Holds the reference to the managed object. */
> >> private final AtomicReference<T> reference = new
> AtomicReference<T>();
> >>
> >> + /** Holds the exception that terminated the initialize() method, if
> >> an exception was thrown */
> >> + private final AtomicReference<ConcurrentException> referenceExc =
> new
> >> AtomicReference<ConcurrentException>();
> >> +
> >> + /** 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);
> >> + }
> >> }
> >> }
> >>
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
> >>
> >> 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 AbstractConcurrent
> >> @Test
> >> public void testGetConcurrent() throws ConcurrentException,
> >> InterruptedException {
> >> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
> >> }
> >>
> >> // 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 AbstractConcurrent
> >> * @return the initializer object to be tested
> >> */
> >> protected abstract ConcurrentInitializer<Object>
> createInitializer();
> >> +
> >> + /**
> >> + * Creates a {@link ConcurrentInitializer} object that always
> throws
> >> + * exceptions.
> >> + *
> >> + * @return
> >> + */
> >> + protected abstract ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer();
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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 exten
> >> }
> >> };
> >> }
> >> +
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer() {
> >> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
> >> + }
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> extends
> >> AtomicSafeInitializer<Object> {
> >> + @Override
> >> + protected Object initialize() throws ConcurrentException {
> >> + throw new ConcurrentException(testExceptionMessage,
> >> testCauseException);
> >> + }
> >> + }
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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);
> >> }
> >>
> >> /**
> >> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
> >> }
> >>
> >> /**
> >> + * Returns the exception-throwing initializer to be tested.
> >> + *
> >> + * @return the {@code AtomicSafeInitializer} under test when
> >> validating
> >> + * exception handling
> >> + */
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> createExceptionThrowingInitializer() {
> >> + return exceptionThrowingInitializer;
> >> + }
> >> +
> >> + /**
> >> * Tests that initialize() is called only once.
> >> *
> >> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
> >> because {@link #testGetConcurrent()} may throw it
> >> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
> >> 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 e
> >> return new Object();
> >> }
> >> }
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingAtomicSafeInitializerTestImpl
> extends
> >> AtomicSafeInitializer<Object> {
> >> + @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<Object> {
> >> + 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<Object> initializer;
> >> + private Object result;
> >> + private boolean caughtException;
> >> +
> >> + public InitializerTestThread(AtomicSafeInitializer<Object>
> >> 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;
> >> + }
> >> + }
> >> }
> >>
> >> Modified:
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> URL:
> >>
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
> >>
> >>
> ==============================================================================
> >> ---
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> (original)
> >> +++
> >>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> >> Mon Feb 23 20:15:49 2015
> >> @@ -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
> >> return initializer;
> >> }
> >>
> >> + @Override
> >> + protected ConcurrentInitializer<Object>
> >> 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
> >> return new Object();
> >> }
> >> }
> >> +
> >> +
> >> + /**
> >> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> >> This
> >> + * implementation always throws an exception.
> >> + */
> >> + private class ExceptionThrowingLazyInitializerTestImpl extends
> >> LazyInitializer<Object> {
> >> + @Override
> >> + protected Object initialize() throws ConcurrentException {
> >> + throw new ConcurrentException(testExceptionMessage,
> >> testCauseException);
> >> + }
> >> + }
> >> }
> >>
> >>
> >>
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src: changes/
main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Oliver Heger <ol...@oliver-heger.de>.
Am 23.02.2015 um 21:35 schrieb Benedikt Ritter:
> Oliver Heger has raised concerns about this commit in JIRA [1]:
>
>> This is a strong change in the behavior of this class. The main property
> of atomic initializers was that they are non
>> blocking. Now a blocking wait has been introduced. When there is so much
> contention that the busy wait is
>> actually a problem, wouln't it then be better to directly use a blocking
> variant like lazy initializer?
>
> I've looked through the JavaDoc of AtomicInitializer once more. It says:
> "Because {@code AtomicSafeInitializer} does not use synchronization at all
> it probably outruns {@link LazyInitializer}, at least under low or moderate
> concurrent access."
>
> This is the only thing I can find regarding concurrency properties of
> AtomicInitializer. I think this still holds, doesn't it?
No, the CountDownLatch is synchronized.
There are multiple initializer implementations with different properties
which are suitable for different use cases and scenarios. The atomic
initializers are useful if you want to avoid blocking calls, but they
might be an inferior solution under high contention.
My fear is that this commit optimizes the class for a use case which can
be served better by another initializer implementation which is already
blocking; and this optimization has negative impact on the original
intention of AtomicSafeInitializer.
Oliver
>
> Benedikt
>
> [1] https://issues.apache.org/jira/browse/LANG-1086
>
> 2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
>
>> Author: britter
>> Date: Mon Feb 23 20:15:49 2015
>> New Revision: 1661762
>>
>> URL: http://svn.apache.org/r1661762
>> Log:
>> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
>> fixes #46 from github. Thanks to github user artnaseef.
>>
>> Modified:
>> commons/proper/lang/trunk/src/changes/changes.xml
>>
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>>
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>>
>> Modified: commons/proper/lang/trunk/src/changes/changes.xml
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
>> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23
>> 20:15:49 2015
>> @@ -22,6 +22,7 @@
>> <body>
>>
>> <release version="3.4" date="tba" description="tba">
>> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
>> wait from AtomicSafeInitializer.get()</action>
>> <action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan
>> Baker">DiffBuilder.append(String, Object left, Object right) does not do a
>> left.equals(right) check</action>
>> <action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan
>> Baker">StrSubstitutor.replaceSystemProperties does not work
>> consistently</action>
>> <action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan
>> Baker">Add option to disable the "objectsTriviallyEqual" test in
>> DiffBuilder</action>
>>
>> Modified:
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> (original)
>> +++
>> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>> Mon Feb 23 20:15:49 2015
>> @@ -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 AtomicSafeInitiali
>> /** Holds the reference to the managed object. */
>> private final AtomicReference<T> reference = new AtomicReference<T>();
>>
>> + /** Holds the exception that terminated the initialize() method, if
>> an exception was thrown */
>> + private final AtomicReference<ConcurrentException> referenceExc = new
>> AtomicReference<ConcurrentException>();
>> +
>> + /** 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);
>> + }
>> }
>> }
>>
>>
>> Modified:
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> (original)
>> +++
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>> Mon Feb 23 20:15:49 2015
>> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>>
>> 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 AbstractConcurrent
>> @Test
>> public void testGetConcurrent() throws ConcurrentException,
>> InterruptedException {
>> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
>> }
>>
>> // 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 AbstractConcurrent
>> * @return the initializer object to be tested
>> */
>> protected abstract ConcurrentInitializer<Object> createInitializer();
>> +
>> + /**
>> + * Creates a {@link ConcurrentInitializer} object that always throws
>> + * exceptions.
>> + *
>> + * @return
>> + */
>> + protected abstract ConcurrentInitializer<Object>
>> createExceptionThrowingInitializer();
>> }
>>
>> Modified:
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> (original)
>> +++
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>> Mon Feb 23 20:15:49 2015
>> @@ -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 exten
>> }
>> };
>> }
>> +
>> + @Override
>> + protected ConcurrentInitializer<Object>
>> createExceptionThrowingInitializer() {
>> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
>> + }
>> +
>> + /**
>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>> This
>> + * implementation always throws an exception.
>> + */
>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends
>> AtomicSafeInitializer<Object> {
>> + @Override
>> + protected Object initialize() throws ConcurrentException {
>> + throw new ConcurrentException(testExceptionMessage,
>> testCauseException);
>> + }
>> + }
>> }
>>
>> Modified:
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> (original)
>> +++
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>> Mon Feb 23 20:15:49 2015
>> @@ -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);
>> }
>>
>> /**
>> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
>> }
>>
>> /**
>> + * Returns the exception-throwing initializer to be tested.
>> + *
>> + * @return the {@code AtomicSafeInitializer} under test when
>> validating
>> + * exception handling
>> + */
>> + @Override
>> + protected ConcurrentInitializer<Object>
>> createExceptionThrowingInitializer() {
>> + return exceptionThrowingInitializer;
>> + }
>> +
>> + /**
>> * Tests that initialize() is called only once.
>> *
>> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
>> because {@link #testGetConcurrent()} may throw it
>> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
>> 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 e
>> return new Object();
>> }
>> }
>> +
>> + /**
>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>> This
>> + * implementation always throws an exception.
>> + */
>> + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends
>> AtomicSafeInitializer<Object> {
>> + @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<Object> {
>> + 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<Object> initializer;
>> + private Object result;
>> + private boolean caughtException;
>> +
>> + public InitializerTestThread(AtomicSafeInitializer<Object>
>> 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;
>> + }
>> + }
>> }
>>
>> Modified:
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>>
>> ==============================================================================
>> ---
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> (original)
>> +++
>> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>> Mon Feb 23 20:15:49 2015
>> @@ -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
>> return initializer;
>> }
>>
>> + @Override
>> + protected ConcurrentInitializer<Object>
>> 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
>> return new Object();
>> }
>> }
>> +
>> +
>> + /**
>> + * A concrete test implementation of {@code AtomicSafeInitializer}.
>> This
>> + * implementation always throws an exception.
>> + */
>> + private class ExceptionThrowingLazyInitializerTestImpl extends
>> LazyInitializer<Object> {
>> + @Override
>> + protected Object initialize() throws ConcurrentException {
>> + throw new ConcurrentException(testExceptionMessage,
>> testCauseException);
>> + }
>> + }
>> }
>>
>>
>>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1661762 - in /commons/proper/lang/trunk/src:
changes/ main/java/org/apache/commons/lang3/concurrent/ test/java/org/apache/commons/lang3/concurrent/
Posted by Benedikt Ritter <br...@apache.org>.
Oliver Heger has raised concerns about this commit in JIRA [1]:
> This is a strong change in the behavior of this class. The main property
of atomic initializers was that they are non
> blocking. Now a blocking wait has been introduced. When there is so much
contention that the busy wait is
> actually a problem, wouln't it then be better to directly use a blocking
variant like lazy initializer?
I've looked through the JavaDoc of AtomicInitializer once more. It says:
"Because {@code AtomicSafeInitializer} does not use synchronization at all
it probably outruns {@link LazyInitializer}, at least under low or moderate
concurrent access."
This is the only thing I can find regarding concurrency properties of
AtomicInitializer. I think this still holds, doesn't it?
Benedikt
[1] https://issues.apache.org/jira/browse/LANG-1086
2015-02-23 21:15 GMT+01:00 <br...@apache.org>:
> Author: britter
> Date: Mon Feb 23 20:15:49 2015
> New Revision: 1661762
>
> URL: http://svn.apache.org/r1661762
> Log:
> LANG-1086: Remove busy wait from AtomicSafeInitializer.get(). This also
> fixes #46 from github. Thanks to github user artnaseef.
>
> Modified:
> commons/proper/lang/trunk/src/changes/changes.xml
>
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
>
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
>
> Modified: commons/proper/lang/trunk/src/changes/changes.xml
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/changes/changes.xml?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> --- commons/proper/lang/trunk/src/changes/changes.xml [utf-8] (original)
> +++ commons/proper/lang/trunk/src/changes/changes.xml [utf-8] Mon Feb 23
> 20:15:49 2015
> @@ -22,6 +22,7 @@
> <body>
>
> <release version="3.4" date="tba" description="tba">
> + <action issue="LANG-1086" type="update" dev="britter">Remove busy
> wait from AtomicSafeInitializer.get()</action>
> <action issue="LANG-1081" type="fix" dev="britter" due-to="Jonathan
> Baker">DiffBuilder.append(String, Object left, Object right) does not do a
> left.equals(right) check</action>
> <action issue="LANG-1055" type="fix" dev="britter" due-to="Jonathan
> Baker">StrSubstitutor.replaceSystemProperties does not work
> consistently</action>
> <action issue="LANG-1082" type="add" dev="britter" due-to="Jonathan
> Baker">Add option to disable the "objectsTriviallyEqual" test in
> DiffBuilder</action>
>
> Modified:
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> ---
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> (original)
> +++
> commons/proper/lang/trunk/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java
> Mon Feb 23 20:15:49 2015
> @@ -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 AtomicSafeInitiali
> /** Holds the reference to the managed object. */
> private final AtomicReference<T> reference = new AtomicReference<T>();
>
> + /** Holds the exception that terminated the initialize() method, if
> an exception was thrown */
> + private final AtomicReference<ConcurrentException> referenceExc = new
> AtomicReference<ConcurrentException>();
> +
> + /** 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);
> + }
> }
> }
>
>
> Modified:
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> ---
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> (original)
> +++
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerTest.java
> Mon Feb 23 20:15:49 2015
> @@ -18,6 +18,8 @@ package org.apache.commons.lang3.concurr
>
> 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 AbstractConcurrent
> @Test
> public void testGetConcurrent() throws ConcurrentException,
> InterruptedException {
> - final ConcurrentInitializer<Object> 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<Object> 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 AbstractConcurrent
> }
>
> // 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 AbstractConcurrent
> * @return the initializer object to be tested
> */
> protected abstract ConcurrentInitializer<Object> createInitializer();
> +
> + /**
> + * Creates a {@link ConcurrentInitializer} object that always throws
> + * exceptions.
> + *
> + * @return
> + */
> + protected abstract ConcurrentInitializer<Object>
> createExceptionThrowingInitializer();
> }
>
> Modified:
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> ---
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> (original)
> +++
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicInitializerTest.java
> Mon Feb 23 20:15:49 2015
> @@ -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 exten
> }
> };
> }
> +
> + @Override
> + protected ConcurrentInitializer<Object>
> createExceptionThrowingInitializer() {
> + return new ExceptionThrowingAtomicSafeInitializerTestImpl();
> + }
> +
> + /**
> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> This
> + * implementation always throws an exception.
> + */
> + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends
> AtomicSafeInitializer<Object> {
> + @Override
> + protected Object initialize() throws ConcurrentException {
> + throw new ConcurrentException(testExceptionMessage,
> testCauseException);
> + }
> + }
> }
>
> Modified:
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> ---
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> (original)
> +++
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java
> Mon Feb 23 20:15:49 2015
> @@ -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);
> }
>
> /**
> @@ -49,6 +60,17 @@ public class AtomicSafeInitializerTest e
> }
>
> /**
> + * Returns the exception-throwing initializer to be tested.
> + *
> + * @return the {@code AtomicSafeInitializer} under test when
> validating
> + * exception handling
> + */
> + @Override
> + protected ConcurrentInitializer<Object>
> createExceptionThrowingInitializer() {
> + return exceptionThrowingInitializer;
> + }
> +
> + /**
> * Tests that initialize() is called only once.
> *
> * @throws org.apache.commons.lang3.concurrent.ConcurrentException
> because {@link #testGetConcurrent()} may throw it
> @@ -62,6 +84,92 @@ public class AtomicSafeInitializerTest e
> 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 e
> return new Object();
> }
> }
> +
> + /**
> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> This
> + * implementation always throws an exception.
> + */
> + private class ExceptionThrowingAtomicSafeInitializerTestImpl extends
> AtomicSafeInitializer<Object> {
> + @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<Object> {
> + 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<Object> initializer;
> + private Object result;
> + private boolean caughtException;
> +
> + public InitializerTestThread(AtomicSafeInitializer<Object>
> 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;
> + }
> + }
> }
>
> Modified:
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java?rev=1661762&r1=1661761&r2=1661762&view=diff
>
> ==============================================================================
> ---
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> (original)
> +++
> commons/proper/lang/trunk/src/test/java/org/apache/commons/lang3/concurrent/LazyInitializerTest.java
> Mon Feb 23 20:15:49 2015
> @@ -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
> return initializer;
> }
>
> + @Override
> + protected ConcurrentInitializer<Object>
> 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
> return new Object();
> }
> }
> +
> +
> + /**
> + * A concrete test implementation of {@code AtomicSafeInitializer}.
> This
> + * implementation always throws an exception.
> + */
> + private class ExceptionThrowingLazyInitializerTestImpl extends
> LazyInitializer<Object> {
> + @Override
> + protected Object initialize() throws ConcurrentException {
> + throw new ConcurrentException(testExceptionMessage,
> testCauseException);
> + }
> + }
> }
>
>
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter