diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/Decoder.java b/archaius2-api/src/main/java/com/netflix/archaius/api/Decoder.java index 70ef8f47..6d421650 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/Decoder.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/Decoder.java @@ -20,6 +20,9 @@ /** * API for decoding properties to arbitrary types. * + * @implSpec Implementations of this interface MUST be idempotent. Failing to do so will result in correctness errors. + * Implementations of this interface SHOULD also be cheap to execute. Expensive or blocking operations are to be + * avoided since they can potentially cause large delays in property resolution. * @author spencergibb */ public interface Decoder { diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java b/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java index 037b9497..03e1a94c 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/Property.java @@ -20,7 +20,7 @@ import java.util.function.Supplier; /** - * API for composeable property access with optional chaining with default value support + * API for composable property access with optional chaining with default value support * as well as change notification. * * A {@link PropertyRepository} implementation normally implements some level of caching @@ -44,15 +44,15 @@ */ public interface Property extends Supplier { /** - * Token returned when calling onChange through which change notification can be - * unsubscribed. + * Token returned when calling {@link #subscribe(Consumer)} or {@link #onChange(Consumer)} through which change + * notification can be unsubscribed. */ interface Subscription { void unsubscribe(); } /** - * Return the most recent value of the property. + * Return the most recent value of the property. * * @return Most recent value for the property */ @@ -60,32 +60,30 @@ interface Subscription { T get(); /** - * Add a listener that will be called whenever the property value changes - * @param listener + * Add a listener that will be called whenever the property value changes. + * @implNote Implementors of this interface MUST override this method or {@link #subscribe(Consumer)}. + * @deprecated Use {@link Property#subscribe(Consumer)} instead */ @Deprecated default void addListener(PropertyListener listener) { - onChange(new Consumer() { - @Override - public void accept(T t) { - listener.accept(t); - } - }); + // Call subscribe for backwards compatibility. + // TODO: This behavior should be removed soon, because it causes a loop that implementors must work around. + subscribe(listener); } /** * Remove a listener previously registered by calling addListener - * @param listener + * @deprecated Use the {@link Subscription} object returned by {@link Property#subscribe(Consumer)} instead. */ @Deprecated default void removeListener(PropertyListener listener) {} /** - * @deprecated Use {@link Property#subscribe(Consumer)} - * @param consumer + * @deprecated Use {@link Property#subscribe(Consumer)} instead. */ @Deprecated default Subscription onChange(Consumer consumer) { + // Call subscribe for backwards compatibility return subscribe(consumer); } @@ -95,17 +93,13 @@ default Subscription onChange(Consumer consumer) { * since the notification only applies to the state of the chained property * up until this point. Changes to subsequent Property objects returned from {@link Property#orElse} * or {@link Property#map(Function)} will not trigger calls to this consumer. - - * @param consumer + * + * @implNote Implementors of this interface MUST override this method or {@link #addListener(PropertyListener)} + * to break a circular loop between the default implementations. * @return Subscription that may be unsubscribed to no longer get change notifications */ default Subscription subscribe(Consumer consumer) { - PropertyListener listener = new PropertyListener() { - @Override - public void onChange(T value) { - consumer.accept(value); - } - }; + PropertyListener listener = (PropertyListener) consumer; addListener(listener); return () -> removeListener(listener); @@ -114,7 +108,6 @@ public void onChange(T value) { /** * Create a new Property object that will return the specified defaultValue if * this object's property is not found. - * @param defaultValue * @return Newly constructed Property object */ default Property orElse(T defaultValue) { @@ -125,7 +118,6 @@ default Property orElse(T defaultValue) { * Create a new Property object that will fetch the property backed by the provided * key. The return value of the supplier will be cached until the configuration has changed * - * @param key * @return Newly constructed Property object */ default Property orElseGet(String key) { @@ -139,7 +131,6 @@ default Property orElseGet(String key) { * * Note that no orElseGet() calls may be made on a mapped property * - * @param delegate * @return Newly constructed Property object */ default Property map(Function mapper) { diff --git a/archaius2-api/src/main/java/com/netflix/archaius/api/StrInterpolator.java b/archaius2-api/src/main/java/com/netflix/archaius/api/StrInterpolator.java index 5278a6f4..a3267a59 100644 --- a/archaius2-api/src/main/java/com/netflix/archaius/api/StrInterpolator.java +++ b/archaius2-api/src/main/java/com/netflix/archaius/api/StrInterpolator.java @@ -26,7 +26,11 @@ * interpolator.create(lookup).resolve("123-${foo}") -> 123-abc * } * - * + * + * @implSpec Implementations of this interface MUST be idempotent (as long as the backing Config remains unchanged). + * Failing to do so will result in correctness errors. + * Implementations of this interface SHOULD also be cheap to execute. Expensive or blocking operations are to be + * avoided since they can potentially cause large delays in property resolution. * @author elandau * */ @@ -38,7 +42,7 @@ public interface StrInterpolator { * * @author elandau */ - public interface Lookup { + interface Lookup { String lookup(String key); } @@ -47,14 +51,11 @@ public interface Lookup { * @author elandau * */ - public interface Context { + interface Context { /** * Resolve a string with replaceable variables using the provided map to lookup replacement * values. The implementation should deal with nested replacements and throw an exception * for infinite recursion. - * - * @param value - * @return */ String resolve(String value); } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java b/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java index 09bd110d..05655b0c 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/AbstractProperty.java @@ -3,6 +3,11 @@ import com.netflix.archaius.api.Property; import com.netflix.archaius.api.PropertyListener; +/** @deprecated This class has no known users and doesn't offer any actual advantage over implementing {@link Property} + * directly. Scheduled to be removed by the end of 2025. + **/ +@Deprecated +// TODO Remove by the end of 2025 public abstract class AbstractProperty implements Property { private final String key; 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 5cd3c771..96e52f76 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultPropertyFactory.java @@ -13,19 +13,25 @@ import java.lang.reflect.Type; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicStampedReference; +import java.util.concurrent.atomic.AtomicReferenceFieldUpdater; import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; public class DefaultPropertyFactory implements PropertyFactory, ConfigListener { private static final Logger LOG = LoggerFactory.getLogger(DefaultPropertyFactory.class); + + @SuppressWarnings("rawtypes") + private static final AtomicReferenceFieldUpdater CACHED_VALUE_UPDATER + = AtomicReferenceFieldUpdater.newUpdater(PropertyImpl.class, CachedValue.class, "cachedValue"); /** * Create a Property factory that is attached to a specific config @@ -68,83 +74,7 @@ public DefaultPropertyFactory(Config config) { @Deprecated @SuppressWarnings("deprecation") public PropertyContainer getProperty(String propName) { - return new PropertyContainer() { - @Override - public Property asString(String defaultValue) { - return get(propName, String.class).orElse(defaultValue); - } - - @Override - public Property asInteger(Integer defaultValue) { - return get(propName, Integer.class).orElse(defaultValue); - } - - @Override - public Property asLong(Long defaultValue) { - return get(propName, Long.class).orElse(defaultValue); - } - - @Override - public Property asDouble(Double defaultValue) { - return get(propName, Double.class).orElse(defaultValue); - } - - @Override - public Property asFloat(Float defaultValue) { - return get(propName, Float.class).orElse(defaultValue); - } - - @Override - public Property asShort(Short defaultValue) { - return get(propName, Short.class).orElse(defaultValue); - } - - @Override - public Property asByte(Byte defaultValue) { - return get(propName, Byte.class).orElse(defaultValue); - } - - @Override - public Property asBoolean(Boolean defaultValue) { - return get(propName, Boolean.class).orElse(defaultValue); - } - - @Override - public Property asBigDecimal(BigDecimal defaultValue) { - return get(propName, BigDecimal.class).orElse(defaultValue); - } - - @Override - public Property asBigInteger(BigInteger defaultValue) { - return get(propName, BigInteger.class).orElse(defaultValue); - } - - @Override - public Property asType(Class type, T defaultValue) { - return get(propName, type).orElse(defaultValue); - } - - @Override - public Property asType(Function mapper, String defaultValue) { - T typedDefaultValue = applyOrThrow(mapper, defaultValue); - return getFromSupplier(propName, null, () -> { - String value = config.getString(propName, null); - if (value != null) { - return applyOrThrow(mapper, value); - } - - return typedDefaultValue; - }); - } - - private T applyOrThrow(Function mapper, String value) { - try { - return mapper.apply(value); - } catch (RuntimeException e) { - throw new ParseException("Invalid value '" + value + "' for property '" + propName + "'.", e); - } - } - }; + return new PropertyContainerImpl(propName); } @Override @@ -201,42 +131,86 @@ private Property getFromSupplier(KeyAndType keyAndType, Supplier su return (Property) properties.computeIfAbsent(keyAndType, (ignore) -> new PropertyImpl<>(keyAndType, supplier)); } + /** + * Implementation of the Property interface. This class looks at the factory's masterVersion on each read to + * determine if the cached parsed values is stale. + */ private final class PropertyImpl implements Property { + private final KeyAndType keyAndType; private final Supplier supplier; - private final AtomicStampedReference cache = new AtomicStampedReference<>(null, -1); - private final ConcurrentMap, Subscription> oldSubscriptions = new ConcurrentHashMap<>(); + + // This field cannot be private because it's accessed via reflection in the CACHED_VALUE_UPDATER :-( + volatile CachedValue cachedValue; + + // Keep track of old-style listeners so we can unsubscribe them when they are removed + // Field is initialized on demand only if it's actually needed. + // Access is synchronized on _this_. + private Map, Subscription> oldSubscriptions; public PropertyImpl(KeyAndType keyAndType, Supplier supplier) { this.keyAndType = keyAndType; this.supplier = supplier; } - + + /** + * Get the current value of the property. If the value is not cached or the cache is stale, the value is + * updated from the supplier. If the supplier throws an exception, the exception is logged and rethrown. + *

+ * This method is intended to provide the following semantics: + *

    + *
  • Changes to a property are atomic.
    • + *
  • Updates from the backing Config are eventually consistent.
    • + *
  • When multiple updates happen then "last one wins", as ordered by calls to the PropertyFactory's invalidate() method.
    • + *
  • A property only changes value *after* a call to invalidate()
    • + *
  • Updates *across* different properties are not transactional. A thread may see (newA, oldB) while a different concurrent thread sees (oldA, newB)
    • + *
+ * @throws RuntimeException if the supplier throws an exception + */ @Override public T get() { - int[] cacheVersion = new int[1]; - T currentValue = cache.get(cacheVersion); - int latestVersion = masterVersion.get(); - - if (cacheVersion[0] == latestVersion) { - return currentValue; + int currentMasterVersion = masterVersion.get(); + CachedValue currentCachedValue = this.cachedValue; + + // Happy path. We have an up-to-date cached value, so just return that. + // We check for >= in case an upstream update happened between getting the version and the cached value AND + // another thread came and updated the cache. + if (currentCachedValue != null && currentCachedValue.version >= currentMasterVersion) { + return currentCachedValue.value; } + // No valid cache, let's try to update it. Multiple threads may get here and try to update. That's fine, + // the worst case is wasted effort. A hidden assumption here is that the supplier is idempotent and relatively + // cheap, which should be true unless the user installed badly behaving interpolators or converters in + // the Config object. + // The tricky edge case is if another update came in between the check above to get the version and + // the call to the supplier. In that case we'll tag the updated value with an old version number. That's fine, + // since the next call to get() will see the old version and try again. try { - T newValue = supplier.get(); - - if (cache.compareAndSet(currentValue, newValue, cacheVersion[0], latestVersion)) { - // newValue could be stale here already, if the cache was updated *again* between the CAS and this line - // We don't care enough about this edge case to fix it. - return newValue; - } + // Get the new value from the supplier. This call could fail. + CachedValue newValue = new CachedValue<>(supplier.get(), currentMasterVersion); + + /* + * We successfully got the new value, so now we update the cache. We use an atomic CAS operation to guard + * from edge cases where another thread could have updated to a higher version than we have, in a flow like this: + * Assume currentVersion started at 1., property cache is set to 1 too. + * 1. Upstream update bumps version to 2. + * 2. Thread A reads currentVersion at 2, cachedValue at 1, proceeds to start update, gets interrupted and yields the cpu. + * 3. Thread C bumps version to 3, yields the cpu. + * 4. Thread B is scheduled, reads currentVersion at 3, cachedValue still at 1, proceeds to start update. + * 5. Thread B keeps running, updates cache to 3, yields. + * 6. Thread A resumes, tries to write cache with version 2. + */ + CACHED_VALUE_UPDATER.compareAndSet(this, currentCachedValue, newValue); + + return newValue.value; } catch (RuntimeException e) { + // Oh, no, something went wrong while trying to get the new value. Log the error and rethrow the exception + // so our caller knows there's a problem. We leave the cache unchanged. Next caller will try again. LOG.error("Unable to get current version of property '{}'", keyAndType.key, e); - throw e; // Rethrow the exception, our caller should know that the property is not available + throw e; } - - return cache.getReference(); } @Override @@ -273,8 +247,11 @@ public synchronized void run() { @Deprecated @Override @SuppressWarnings("deprecation") - public void addListener(PropertyListener listener) { - oldSubscriptions.put(listener, onChange(listener)); + public synchronized void addListener(PropertyListener listener) { + if (oldSubscriptions == null) { + oldSubscriptions = new HashMap<>(); + } + oldSubscriptions.put(listener, subscribe(listener)); } /** @@ -284,7 +261,11 @@ public void addListener(PropertyListener listener) { @Deprecated @Override @SuppressWarnings("deprecation") - public void removeListener(PropertyListener listener) { + public synchronized void removeListener(PropertyListener listener) { + if (oldSubscriptions == null) { + return; + } + Subscription subscription = oldSubscriptions.remove(listener); if (subscription != null) { subscription.unsubscribe(); @@ -326,10 +307,14 @@ public Property map(Function mapper) { @Override public String toString() { - return "Property [Key=" + getKey() + "; value="+get() + "]"; + return "Property [Key=" + keyAndType + "; cachedValue="+ cachedValue + "]"; } } + /** + * Holder for a pair of property name and type. Used as a key in the properties map. + * @param + */ private static final class KeyAndType { private final String key; private final Type type; @@ -384,4 +369,116 @@ public String toString() { '}'; } } + + /** A holder for a cached value and the version of the master config at which it was updated. */ + private static final class CachedValue { + final T value; + final int version; + + CachedValue(T value, int version) { + this.value = value; + this.version = version; + } + + @Override + public String toString() { + return "CachedValue{" + + "value=" + value + + ", version=" + version + + '}'; + } + } + + /** + * Implements the deprecated PropertyContainer interface, for backwards compatibility. + */ + @SuppressWarnings("deprecation") + private final class PropertyContainerImpl implements PropertyContainer { + private final String propName; + + public PropertyContainerImpl(String propName) { + this.propName = propName; + } + + @Override + public Property asString(String defaultValue) { + return get(propName, String.class).orElse(defaultValue); + } + + @Override + public Property asInteger(Integer defaultValue) { + return get(propName, Integer.class).orElse(defaultValue); + } + + @Override + public Property asLong(Long defaultValue) { + return get(propName, Long.class).orElse(defaultValue); + } + + @Override + public Property asDouble(Double defaultValue) { + return get(propName, Double.class).orElse(defaultValue); + } + + @Override + public Property asFloat(Float defaultValue) { + return get(propName, Float.class).orElse(defaultValue); + } + + @Override + public Property asShort(Short defaultValue) { + return get(propName, Short.class).orElse(defaultValue); + } + + @Override + public Property asByte(Byte defaultValue) { + return get(propName, Byte.class).orElse(defaultValue); + } + + @Override + public Property asBoolean(Boolean defaultValue) { + return get(propName, Boolean.class).orElse(defaultValue); + } + + @Override + public Property asBigDecimal(BigDecimal defaultValue) { + return get(propName, BigDecimal.class).orElse(defaultValue); + } + + @Override + public Property asBigInteger(BigInteger defaultValue) { + return get(propName, BigInteger.class).orElse(defaultValue); + } + + @Override + public Property asType(Class type, T defaultValue) { + return get(propName, type).orElse(defaultValue); + } + + @Override + public Property asType(Function mapper, String defaultValue) { + T typedDefaultValue = applyOrThrow(mapper, defaultValue); + return getFromSupplier(propName, null, () -> { + String value = config.getString(propName, null); + if (value != null) { + return applyOrThrow(mapper, value); + } + + return typedDefaultValue; + }); + } + + private T applyOrThrow(Function mapper, String value) { + try { + return mapper.apply(value); + } catch (RuntimeException e) { + throw new ParseException("Invalid value '" + value + "' for property '" + propName + "'.", e); + } + } + + @Override + public String toString() { + return "PropertyContainer [name=" + propName + "]"; + } + } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java b/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java index 5c18d407..9be1c8d9 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/DelegatingProperty.java @@ -3,6 +3,12 @@ import com.netflix.archaius.api.Property; import com.netflix.archaius.api.PropertyListener; +/** + * Base class for Property implementations that delegate to another Property. + * @deprecated There are no known implementations of this class. To be removed by the end of 2025 + */ +@Deprecated +// TODO Remove by the end of 2025 public abstract class DelegatingProperty implements Property { protected Property delegate;