diff --git a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java index 12495c1a..6a400452 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/config/AbstractConfig.java @@ -25,6 +25,8 @@ import com.netflix.archaius.exceptions.ParseException; import com.netflix.archaius.interpolate.CommonsStrInterpolator; import com.netflix.archaius.interpolate.ConfigStrLookup; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.lang.reflect.Type; import java.math.BigDecimal; @@ -38,6 +40,7 @@ public abstract class AbstractConfig implements Config { + private static final Logger log = LoggerFactory.getLogger(AbstractConfig.class); private final CopyOnWriteArrayList listeners = new CopyOnWriteArrayList<>(); private final Lookup lookup; private Decoder decoder; @@ -109,27 +112,59 @@ public void removeListener(ConfigListener listener) { listeners.remove(listener); } + /** + * Notify all listeners that the child configuration has been updated. + * @implNote This implementation calls listeners in the order they were added. This is NOT part of the API contract. + * @implSpec Implementors must make sure that if a listener fails it will not prevent other listeners from being + * called. In practice this means that exceptions must be caught (and probably logged), but not rethrown. + */ protected void notifyConfigUpdated(Config child) { for (ConfigListener listener : listeners) { - listener.onConfigUpdated(child); + callSafely(() -> listener.onConfigUpdated(child)); } } + /** + * Notify all listeners that the child configuration encountered an error + * @implNote This implementation calls listeners in the order they were added. This is NOT part of the API contract. + * @implSpec Implementors must make sure that if a listener fails it will not prevent other listeners from being + * called. In practice this means that exceptions must be caught (and probably logged), but not rethrown. + */ protected void notifyError(Throwable t, Config child) { for (ConfigListener listener : listeners) { - listener.onError(t, child); + callSafely(() -> listener.onError(t, child)); } } + /** + * Notify all listeners that a child configuration has been added. + * @implNote This implementation calls listeners in the order they were added. This is NOT part of the API contract. + * @implSpec Implementors must make sure that if a listener fails it will not prevent other listeners from being + * called. In practice this means that exceptions must be caught (and probably logged), but not rethrown. + */ protected void notifyConfigAdded(Config child) { for (ConfigListener listener : listeners) { - listener.onConfigAdded(child); + callSafely(() -> listener.onConfigAdded(child)); } } + /** + * Notify all listeners that the child configuration has been removed. + * @implNote This implementation calls listeners in the order they were added. This is NOT part of the API contract. + * @implSpec Implementors must make sure that if a listener fails it will not prevent other listeners from being + * called. In practice this means that exceptions must be caught (and probably logged), but not rethrown. + */ protected void notifyConfigRemoved(Config child) { for (ConfigListener listener : listeners) { - listener.onConfigRemoved(child); + callSafely(() -> listener.onConfigRemoved(child)); + } + } + + private void callSafely(Runnable r) { + try { + r.run(); + } catch (RuntimeException t) { + log.error("A listener on config {} failed when receiving a notification. listener={}", this, r, t); } } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java index 593c00df..8adcf23f 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/AbstractConfigTest.java @@ -24,6 +24,8 @@ import java.util.Map; import java.util.function.BiConsumer; +import com.netflix.archaius.api.Config; +import com.netflix.archaius.api.ConfigListener; import com.netflix.archaius.exceptions.ParseException; import org.junit.jupiter.api.Test; @@ -33,6 +35,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; public class AbstractConfigTest { @@ -181,4 +185,32 @@ public void testGetRawNumerics() { assertEquals(42L, config.get(Long.class, "int")); assertEquals(42L, config.get(Long.class, "byte")); } + + @Test + public void testListeners() { + ConfigListener goodListener = mock(ConfigListener.class, "goodListener"); + ConfigListener alwaysFailsListener = mock(ConfigListener.class, invocation -> { throw new RuntimeException("This listener fails on purpose"); }); + ConfigListener secondGoodListener = mock(ConfigListener.class, "secondGoodListener"); + RuntimeException mockError = new RuntimeException("Mock error"); + + Config mockChildConfig = mock(Config.class); + + config.addListener(alwaysFailsListener); + config.addListener(goodListener); + config.addListener(secondGoodListener); + + config.notifyConfigUpdated(mockChildConfig); + config.notifyConfigAdded(mockChildConfig); + config.notifyConfigRemoved(mockChildConfig); + config.notifyError(mockError, mockChildConfig); + + // All 3 listeners should receive all notifications (In order, actually, but we should not verify that + // because it's not part of the contract). + for (ConfigListener listener : Arrays.asList(goodListener, alwaysFailsListener, secondGoodListener)) { + verify(listener).onConfigUpdated(mockChildConfig); + verify(listener).onConfigAdded(mockChildConfig); + verify(listener).onConfigRemoved(mockChildConfig); + verify(listener).onError(mockError, mockChildConfig); + } + } } diff --git a/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java b/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java index c86b7598..e1e6de73 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/config/MapConfigTest.java @@ -34,6 +34,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +/** This is ALSO a test of many of the methods in the AbstractConfig super class. */ public class MapConfigTest { private final MapConfig config = MapConfig.builder() .put("str", "value")