Ensure that one failing config listener does not prevent others from …

…being called.

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<ConfigListener> 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);
         }
     }
 

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);
+        }
+    }
 }

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")