This is an automated email from the ASF dual-hosted git repository. ggregory pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/commons-lang.git
commit 0f211efaf1228c3371b2a81de095661a94907f5e Author: Gary D. Gregory <[email protected]> AuthorDate: Wed Aug 20 18:14:44 2025 -0400 org.apache.commons.lang3.concurrent.AtomicSafeInitializer.get() can spin internally if the FailableSupplier given to org.apache.commons.lang3.concurrent.AbstractConcurrentInitializer.AbstractBuilder.setInitializer(FailableSupplier) throws a RuntimeException --- src/changes/changes.xml | 1 + .../lang3/concurrent/AtomicSafeInitializer.java | 13 +++++-- ...oncurrentInitializerCloseAndExceptionsTest.java | 15 +++----- .../concurrent/AtomicSafeInitializerTest.java | 44 ++++++++++++++++++++-- 4 files changed, 57 insertions(+), 16 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 7eb7038ba..1e9fe0b76 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -50,6 +50,7 @@ The <action> type attribute can be add,update,fix,remove. <action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.lang3.reflect.MethodUtils.getMethodObject(Class<?>, String, Class<?>...) now returns null instead of throwing a NullPointerException, as it does for other exception types.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">Reduce spurious failures in org.apache.commons.lang3.ArrayUtilsTest methods that test ArrayUtils.shuffle() methods.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">MethodUtils cannot find or invoke a public method on a public class implemented in its package-private superclass.</action> + <action type="fix" dev="ggregory" due-to="Stanislav Fort, Gary Gregory">org.apache.commons.lang3.concurrent.AtomicSafeInitializer.get() can spin internally if the FailableSupplier given to org.apache.commons.lang3.concurrent.AbstractConcurrentInitializer.AbstractBuilder.setInitializer(FailableSupplier) throws a RuntimeException.</action> <!-- FIX Javadoc --> <action type="fix" dev="ggregory" due-to="Gary Gregory">[javadoc] General improvements.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">[javadoc] Fix thrown exception documentation for org.apache.commons.lang3.reflect.MethodUtils.getMethodObject(Class<?>, String, Class<?>...).</action> diff --git a/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java b/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java index cd2672851..dce8cccdb 100644 --- a/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java +++ b/src/main/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializer.java @@ -18,6 +18,7 @@ import java.util.concurrent.atomic.AtomicReference; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.apache.commons.lang3.function.FailableConsumer; import org.apache.commons.lang3.function.FailableSupplier; @@ -125,13 +126,19 @@ private AtomicSafeInitializer(final FailableSupplier<T, ConcurrentException> ini @Override public final T get() throws ConcurrentException { T result; - while ((result = reference.get()) == getNoInit()) { if (factory.compareAndSet(null, this)) { - reference.set(initialize()); + try { + reference.set(initialize()); + } catch (final Throwable t) { + // Allow retry on failure; otherwise callers spin forever. + factory.set(null); + // Rethrow preserving original semantics: unchecked as-is, checked wrapped. + final Throwable checked = ExceptionUtils.throwUnchecked(t); + throw checked instanceof ConcurrentException ? (ConcurrentException) checked : new ConcurrentException(checked); + } } } - return result; } diff --git a/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerCloseAndExceptionsTest.java b/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerCloseAndExceptionsTest.java index 5d58d7c6e..e8a7eb36f 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerCloseAndExceptionsTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/AbstractConcurrentInitializerCloseAndExceptionsTest.java @@ -137,16 +137,13 @@ void testSupplierThrowsCheckedException() { @Test void testSupplierThrowsConcurrentException() { final ConcurrentException concurrentException = new ConcurrentException(); - @SuppressWarnings("unchecked") - final ConcurrentInitializer<CloseableObject> initializer = createInitializerThatThrowsException( - () -> { - if ("test".equals("test")) { - throw concurrentException; - } - return new CloseableObject(); - }, - FailableConsumer.NOP); + final ConcurrentInitializer<CloseableObject> initializer = createInitializerThatThrowsException(() -> { + if ("test".equals("test")) { + throw concurrentException; + } + return new CloseableObject(); + }, FailableConsumer.NOP); try { initializer.get(); fail(); diff --git a/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java b/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java index f4cf6d5e0..94b4a8c3a 100644 --- a/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java +++ b/src/test/java/org/apache/commons/lang3/concurrent/AtomicSafeInitializerTest.java @@ -14,16 +14,26 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package org.apache.commons.lang3.concurrent; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import java.io.IOException; +import java.nio.file.FileSystemException; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.Timeout.ThreadMode; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; /** * Test class for {@code AtomicSafeInitializer} which also serves as a simple example. @@ -37,6 +47,7 @@ class AtomicSafeInitializerTest extends AbstractConcurrentInitializerTest<Object * </p> */ private static final class AtomicSafeInitializerTestImpl extends AtomicSafeInitializer<Object> { + /** A counter for initialize() invocations. */ final AtomicInteger initCounter = new AtomicInteger(); @@ -53,7 +64,7 @@ protected Object initialize() { /** * Returns the initializer to be tested. * - * @return the {@code AtomicSafeInitializer} under test + * @return the {@code AtomicSafeInitializer} under test. */ @Override protected ConcurrentInitializer<Object> createInitializer() { @@ -68,6 +79,7 @@ public void setUp() { @Test void testGetThatReturnsNullFirstTime() throws ConcurrentException { final AtomicSafeInitializer<Object> initializer = new AtomicSafeInitializer<Object>() { + final AtomicBoolean firstRun = new AtomicBoolean(true); @Override @@ -78,16 +90,40 @@ protected Object initialize() { return new Object(); } }; - assertNull(initializer.get()); assertNull(initializer.get()); } + @ParameterizedTest + @ValueSource(classes = { IOException.class, Exception.class, FileSystemException.class, ReflectiveOperationException.class, ConcurrentException.class }) + @Timeout(value = 5, unit = TimeUnit.SECONDS, threadMode = ThreadMode.SAME_THREAD) + void testInitializerThrowsChecked(final Class<Exception> throwableClass) throws ConcurrentException { + final String message = "Initializing"; + final AtomicSafeInitializer<Object> asi = AtomicSafeInitializer.builder().setInitializer(() -> { + throw throwableClass.getConstructor(String.class).newInstance(message); + }).get(); + final String expected = throwableClass.getSimpleName() + ": " + message; + assertEquals(expected, ExceptionUtils.getRootCauseMessage(assertThrows(ConcurrentException.class, asi::get))); + assertEquals(expected, ExceptionUtils.getRootCauseMessage(assertThrows(ConcurrentException.class, asi::get))); + } + + @ParameterizedTest + @ValueSource(classes = { IllegalStateException.class, IllegalArgumentException.class, NullPointerException.class, RuntimeException.class }) + @Timeout(value = 5, unit = TimeUnit.SECONDS, threadMode = ThreadMode.SAME_THREAD) + void testInitializerThrowsUnchecked(final Class<Exception> throwableClass) throws ConcurrentException { + final String message = "Initializing"; + final AtomicSafeInitializer<Object> asi = AtomicSafeInitializer.builder().setInitializer(() -> { + throw throwableClass.getConstructor(String.class).newInstance(message); + }).get(); + assertEquals(message, assertThrows(throwableClass, asi::get).getMessage()); + assertEquals(message, assertThrows(throwableClass, asi::get).getMessage()); + } + /** * Tests that initialize() is called only once. * - * @throws org.apache.commons.lang3.concurrent.ConcurrentException because {@link #testGetConcurrent()} may throw it - * @throws InterruptedException because {@link #testGetConcurrent()} may throw it + * @throws org.apache.commons.lang3.concurrent.ConcurrentException because {@link #testGetConcurrent()} may throw it. + * @throws InterruptedException because {@link #testGetConcurrent()} may throw it. */ @Test void testNumberOfInitializeInvocations() throws ConcurrentException, InterruptedException {
