From b0a78c361215e8f8abe81c5d3cc6f473aca3f7ac Mon Sep 17 00:00:00 2001 From: Eran Landau Date: Thu, 21 Sep 2017 11:16:33 -0700 Subject: [PATCH 1/2] Improve initializing and reseting listeners in the static bridge --- .../bridge/StaticAbstractConfiguration.java | 111 ++++++++++++------ .../StaticBridgeAddConfigurationTest.java | 49 ++++++++ 2 files changed, 123 insertions(+), 37 deletions(-) create mode 100644 archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java diff --git a/archaius2-archaius1-bridge/src/main/java/com/netflix/archaius/bridge/StaticAbstractConfiguration.java b/archaius2-archaius1-bridge/src/main/java/com/netflix/archaius/bridge/StaticAbstractConfiguration.java index 2f070a1a8..0a30c0b34 100644 --- a/archaius2-archaius1-bridge/src/main/java/com/netflix/archaius/bridge/StaticAbstractConfiguration.java +++ b/archaius2-archaius1-bridge/src/main/java/com/netflix/archaius/bridge/StaticAbstractConfiguration.java @@ -1,7 +1,19 @@ package com.netflix.archaius.bridge; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; + +import javax.inject.Inject; +import javax.inject.Singleton; + import org.apache.commons.configuration.AbstractConfiguration; import org.apache.commons.configuration.Configuration; +import org.apache.commons.configuration.event.ConfigurationEvent; import org.apache.commons.configuration.event.ConfigurationListener; import com.netflix.config.AggregatedConfiguration; @@ -11,15 +23,6 @@ import com.netflix.config.DynamicPropertySupport; import com.netflix.config.PropertyListener; -import java.util.Collection; -import java.util.Iterator; -import java.util.List; -import java.util.Set; -import java.util.concurrent.ConcurrentLinkedQueue; - -import javax.inject.Inject; -import javax.inject.Singleton; - /** * @see StaticArchaiusBridgeModule */ @@ -27,12 +30,12 @@ public class StaticAbstractConfiguration extends AbstractConfiguration implements AggregatedConfiguration, DynamicPropertySupport { private static volatile AbstractConfigurationBridge delegate; - private static ConcurrentLinkedQueue pendingListeners = new ConcurrentLinkedQueue<>(); private static final StaticAbstractConfiguration INSTANCE = new StaticAbstractConfiguration(); @Inject - public static void initialize(DeploymentContext context, AbstractConfigurationBridge config) { + public synchronized static void initialize(DeploymentContext context, AbstractConfigurationBridge config) { + reset(); delegate = config; AbstractConfiguration actualConfig = ConfigurationManager.getConfigInstance(); @@ -44,21 +47,66 @@ public static void initialize(DeploymentContext context, AbstractConfigurationBr } DynamicPropertyFactory.initWithConfigurationSource((AbstractConfiguration)INSTANCE); - - PropertyListener listener; - while (null != (listener = pendingListeners.poll())) { - delegate.addConfigurationListener(listener); - } + + // Bridge change notification from the new delegate to any listeners registered on + // this static class. Notifications will be removed if the StaticAbstractConfiguration + // is reset and will reattached to a new bridge should initialize be called again. + config.addConfigurationListener(INSTANCE.forwardingConfigurationListener); + config.addConfigurationListener(INSTANCE.forwardingPropertyListener); } public static AbstractConfiguration getInstance() { return INSTANCE; } - public static void reset() { + public synchronized static void reset() { + if (delegate != null) { + delegate.removeConfigurationListener(INSTANCE.forwardingConfigurationListener); + } delegate = null; } + private final PropertyListener forwardingPropertyListener; + private final ConfigurationListener forwardingConfigurationListener; + private final CopyOnWriteArrayList propertyListeners = new CopyOnWriteArrayList<>(); + + public StaticAbstractConfiguration() { + this.forwardingPropertyListener = new PropertyListener() { + @Override + public void configSourceLoaded(Object source) { + propertyListeners.forEach(listener -> listener.configSourceLoaded(source)); + } + + @Override + public void addProperty(Object source, String name, Object value, boolean beforeUpdate) { + propertyListeners.forEach(listener -> listener.addProperty(source, name, value, beforeUpdate)); + } + + @Override + public void setProperty(Object source, String name, Object value, boolean beforeUpdate) { + propertyListeners.forEach(listener -> listener.setProperty(source, name, value, beforeUpdate)); + } + + @Override + public void clearProperty(Object source, String name, Object value, boolean beforeUpdate) { + propertyListeners.forEach(listener -> listener.clearProperty(source, name, value, beforeUpdate)); + } + + @Override + public void clear(Object source, boolean beforeUpdate) { + propertyListeners.forEach(listener -> listener.clear(source, beforeUpdate)); + } + + }; + + this.forwardingConfigurationListener = new ConfigurationListener() { + @Override + public void configurationChanged(ConfigurationEvent event) { + StaticAbstractConfiguration.this.fireEvent(event.getType(), event.getPropertyName(), event.getPropertyValue(), event.isBeforeUpdate()); + } + }; + } + @Override public boolean isEmpty() { if (delegate == null) { @@ -168,25 +216,14 @@ protected void clearPropertyDirect(String key) { delegate.clearProperty(key); } - @Override - public void addConfigurationListener(PropertyListener expandedPropertyListener) { - if (delegate == null) { - pendingListeners.add(expandedPropertyListener); - } - else { - delegate.addConfigurationListener(expandedPropertyListener); - } - } - - public void addConfigurationListener(ConfigurationListener l) { - delegate.addConfigurationListener(l); - } - - public boolean removeConfigurationListener(ConfigurationListener l) { - return delegate.removeConfigurationListener(l); - } - public Collection getConfigurationListeners() { - return getConfigurationListeners(); - } + List listeners = new ArrayList<>(super.getConfigurationListeners()); + Optional.ofNullable(delegate).ifPresent(d -> listeners.addAll(d.getConfigurationListeners())); + return listeners; + } + + @Override + public void addConfigurationListener(PropertyListener expandedPropertyListener) { + propertyListeners.add(expandedPropertyListener); + } } diff --git a/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java b/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java new file mode 100644 index 000000000..12afc93e1 --- /dev/null +++ b/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java @@ -0,0 +1,49 @@ +package com.netflix.archaius.bridge; + +import javax.inject.Inject; + +import org.apache.commons.configuration.event.ConfigurationEvent; +import org.apache.commons.configuration.event.ConfigurationListener; +import org.junit.Test; +import org.mockito.Mockito; + +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.netflix.archaius.api.config.SettableConfig; +import com.netflix.archaius.api.inject.RuntimeLayer; +import com.netflix.archaius.guice.ArchaiusModule; +import com.netflix.config.ConfigurationManager; + +public class StaticBridgeAddConfigurationTest { + private static ConfigurationListener listener = Mockito.mock(ConfigurationListener.class); + + public static class Foo { + public static void doSomethingStaticy() { + ConfigurationManager.getConfigInstance().addConfigurationListener(listener); + } + } + + @Inject + @RuntimeLayer + SettableConfig settableConfig; + + @Test + public void test() { + Guice.createInjector(new ArchaiusModule(), new StaticArchaiusBridgeModule(), new AbstractModule() { + @Override + protected void configure() { + Foo.doSomethingStaticy(); + this.requestInjection(StaticBridgeAddConfigurationTest.this); + } + }); + + Mockito.verify(listener, Mockito.never()).configurationChanged(Mockito.isA(ConfigurationEvent.class)); + settableConfig.setProperty("foo", "bar"); + Mockito.verify(listener, Mockito.times(2)).configurationChanged(Mockito.isA(ConfigurationEvent.class)); + + StaticArchaiusBridgeModule.resetStaticBridges(); + settableConfig.setProperty("bar", "baz"); + Mockito.verify(listener, Mockito.times(2)).configurationChanged(Mockito.isA(ConfigurationEvent.class)); + + } +} From 49c28f464d16a286f4adfdcd5c8d3f65a78a21c6 Mon Sep 17 00:00:00 2001 From: Eran Landau Date: Thu, 21 Sep 2017 17:53:08 -0700 Subject: [PATCH 2/2] More meaningful test names --- .../bridge/StaticBridgeAddConfigurationTest.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java b/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java index 12afc93e1..0792e485a 100644 --- a/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java +++ b/archaius2-archaius1-bridge/src/test/java/com/netflix/archaius/bridge/StaticBridgeAddConfigurationTest.java @@ -1,7 +1,5 @@ package com.netflix.archaius.bridge; -import javax.inject.Inject; - import org.apache.commons.configuration.event.ConfigurationEvent; import org.apache.commons.configuration.event.ConfigurationListener; import org.junit.Test; @@ -14,11 +12,13 @@ import com.netflix.archaius.guice.ArchaiusModule; import com.netflix.config.ConfigurationManager; +import javax.inject.Inject; + public class StaticBridgeAddConfigurationTest { private static ConfigurationListener listener = Mockito.mock(ConfigurationListener.class); public static class Foo { - public static void doSomethingStaticy() { + public static void addListenerBeforeBridgeInitialization() { ConfigurationManager.getConfigInstance().addConfigurationListener(listener); } } @@ -28,19 +28,21 @@ public static void doSomethingStaticy() { SettableConfig settableConfig; @Test - public void test() { + public void listenerAddedToStaticBeforeStaticInjection() { Guice.createInjector(new ArchaiusModule(), new StaticArchaiusBridgeModule(), new AbstractModule() { @Override protected void configure() { - Foo.doSomethingStaticy(); + Foo.addListenerBeforeBridgeInitialization(); this.requestInjection(StaticBridgeAddConfigurationTest.this); } }); - + + // Verify that the listener is called Mockito.verify(listener, Mockito.never()).configurationChanged(Mockito.isA(ConfigurationEvent.class)); settableConfig.setProperty("foo", "bar"); Mockito.verify(listener, Mockito.times(2)).configurationChanged(Mockito.isA(ConfigurationEvent.class)); + // Listener no longer invoked after reset StaticArchaiusBridgeModule.resetStaticBridges(); settableConfig.setProperty("bar", "baz"); Mockito.verify(listener, Mockito.times(2)).configurationChanged(Mockito.isA(ConfigurationEvent.class));