From 5ef083dbcf57fc0e32ef41dcda8d06e0aa353b3d Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Wed, 2 Oct 2024 11:53:40 -0700 Subject: [PATCH 1/2] Ensure that one failing subscription does not prevent others from being called. --- .../archaius/DefaultPropertyFactory.java | 29 ++--- .../archaius/property/PropertyTest.java | 100 +++++++++++++++++- 2 files changed, 116 insertions(+), 13 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java index 96e52f76..115d7c2a 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java @@ -224,19 +224,24 @@ public Subscription subscribe(Consumer consumer) { private T current = get(); @Override public synchronized void run() { - T newValue = get(); - if (current == newValue && current == null) { - return; - } else if (current == null) { - current = newValue; - } else if (newValue == null) { - current = null; - } else if (current.equals(newValue)) { - return; - } else { - current = newValue; + try { + T newValue = get(); + if (current == newValue && current == null) { + return; + } else if (current == null) { + current = newValue; + } else if (newValue == null) { + current = null; + } else if (current.equals(newValue)) { + return; + } else { + current = newValue; + } + consumer.accept(current); + } catch (RuntimeException e) { + LOG.error("Unable to notify subscriber about update to property '{}'. Subscriber: {}", + keyAndType, consumer, e); } - consumer.accept(current); } }; diff --git a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java index c9fa8578..8bb5fe60 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/property/PropertyTest.java @@ -30,6 +30,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.IntConsumer; import com.netflix.archaius.api.PropertyContainer; import com.netflix.archaius.exceptions.ParseException; @@ -464,7 +465,104 @@ public void chainedPropertyBadValue() { containsString("'first'"), containsString("'bad'"))); } - + + @Test + public void testSubscriptionsBadValue() { + config.setProperty("first", "2"); + Property integerProperty = factory + .get("first", Integer.class) + .orElse(3); + Property stringProperty = factory.get("first", String.class); + + Consumer integerConsumer = unexpected -> fail("Consumer should not be called. Received argument: " + unexpected); + AtomicReference stringConsumer = new AtomicReference<>(); + + // Order is important here! We want the string subscription to be called *after* trying to call the integer + // subscription, which should fail because "a" can not be decoded as an integer + integerProperty.subscribe(integerConsumer); + stringProperty.subscribe(stringConsumer::set); + + // Initial value for the property is 2 + assertEquals(2, integerProperty.get().intValue()); + assertEquals("2", stringProperty.get()); + + // Set it to something that's not an integer + config.setProperty("first", "a"); + + // The integer consumer is never called + // The string consumer *is* called with the new value + assertEquals("a", stringConsumer.get()); + } + + @Test + public void testSubscriptionsNullValue() { + config.setProperty("first", "2"); + Property integerProperty = factory.get("first", Integer.class); + Property intWithDefaultProperty = factory.get("first", Integer.class).orElse(3); + Property stringProperty = factory.get("first", String.class); + + IntConsumer primitiveIntConsumer = unexpected -> fail("Consumer should not be called. Received argument: " + unexpected); + AtomicInteger primitiveIntConsumerForPropertyWithDefault = new AtomicInteger(1000); + AtomicReference boxedIntConsumer = new AtomicReference<>(1000); + AtomicReference stringConsumer = new AtomicReference<>("initial"); + + // Order is important here! We want the string subscription to be called *after* trying to call the integer + // subscription, which should fail because null can't be cast to a primitive int. + integerProperty.subscribe(primitiveIntConsumer::accept); + intWithDefaultProperty.subscribe(primitiveIntConsumerForPropertyWithDefault::set); + integerProperty.subscribe(boxedIntConsumer::set); + stringProperty.subscribe(stringConsumer::set); + + // Initial value for the property is 2 + assertEquals(2, integerProperty.get().intValue()); + assertEquals("2", stringProperty.get()); + + // Set it to something that's not an integer + config.setProperty("first", null); + + // The integer consumer is never called + // The string consumer *is* called with the new value + assertNull(stringConsumer.get()); + // ... the boxed integer consumer also gets the new value + assertNull(boxedIntConsumer.get()); + // ... finally, the consumer for the property with a default is also called and receives the default value + assertEquals(3, primitiveIntConsumerForPropertyWithDefault.get()); + } + + @Test + public void testSubscriptionsConsumerFails() { + config.setProperty("first", "2"); + Property integerProperty = factory + .get("first", Integer.class) + .orElse(3); + Property stringProperty = factory.get("first", String.class); + + AtomicReference integerConsumerArgument = new AtomicReference<>(); + Consumer faultyConsumer = value -> { + integerConsumerArgument.set(value); // Save argument for later verification + throw new RuntimeException("I'm a bad consumer"); + }; + AtomicReference stringConsumer = new AtomicReference<>(); + + // Order is important here! We want the string subscription to be called *after* trying to call the integer + // subscription, which should fail because "a" can not be decoded as an integer + integerProperty.subscribe(faultyConsumer); + stringProperty.subscribe(stringConsumer::set); + + // Initial value for the property is 2 + assertEquals(2, integerProperty.get().intValue()); + assertEquals("2", stringProperty.get()); + + // Set it to something that's not an integer + config.setProperty("first", "5"); + + // The faulty consumer was called + assertEquals(5, integerConsumerArgument.get().intValue()); + // The string consumer also was called. + assertEquals("5", stringConsumer.get()); + } + + @Test public void testCache() { config.setProperty("foo", "1"); From a3bdca94aef2ec3690f13afdd9debe2469f44b0c Mon Sep 17 00:00:00 2001 From: Rebeca Gallardo Date: Thu, 3 Oct 2024 10:12:42 -0700 Subject: [PATCH 2/2] Ensure that one failing config listener does not prevent others from being called. --- .../archaius/config/AbstractConfig.java | 43 +++++++++++++++++-- .../archaius/config/AbstractConfigTest.java | 32 ++++++++++++++ .../archaius/config/MapConfigTest.java | 1 + 3 files changed, 72 insertions(+), 4 deletions(-) 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")