diff --git a/archaius2-core/build.gradle b/archaius2-core/build.gradle index a11fec65..ac656b06 100644 --- a/archaius2-core/build.gradle +++ b/archaius2-core/build.gradle @@ -19,7 +19,7 @@ apply plugin: 'java-library' dependencies { api project(':archaius2-api') implementation 'org.slf4j:slf4j-api:1.7.36' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' implementation 'commons-codec:commons-codec:1.16.0' testImplementation 'com.google.code.findbugs:jsr305:3.0.1' } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java index 04516926..bb3171a3 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/ConfigProxyFactory.java @@ -10,16 +10,11 @@ import com.netflix.archaius.api.annotations.PropertyName; import com.netflix.archaius.util.Maps; import org.apache.commons.lang3.StringUtils; -import org.apache.commons.lang3.SystemUtils; import org.apache.commons.lang3.text.StrSubstitutor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.inject.Inject; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; -import java.lang.reflect.Constructor; import java.lang.reflect.InvocationHandler; import java.lang.reflect.Method; import java.lang.reflect.Proxy; @@ -244,33 +239,18 @@ else if ("toString".equals(method.getName())) { } final Class returnType = m.getReturnType(); - - Function defaultSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null); + // The proper type for this would be Function but we can't express that in Java. + final Function defaultSupplier; if (m.getAnnotation(DefaultValue.class) != null) { - if (m.isDefault()) { - throw new IllegalArgumentException("@DefaultValue cannot be defined on a method with a default implementation for method " - + m.getDeclaringClass().getName() + "#" + m.getName()); - } else if ( - Map.class.isAssignableFrom(returnType) || - List.class.isAssignableFrom(returnType) || - Set.class.isAssignableFrom(returnType) ) { - throw new IllegalArgumentException("@DefaultValue cannot be used with collections. Use default method implemenation instead " - + m.getDeclaringClass().getName() + "#" + m.getName()); - } - - String value = m.getAnnotation(DefaultValue.class).value(); - if (returnType == String.class) { - defaultSupplier = memoize((T) config.resolve(value)); - } else { - defaultSupplier = memoize(decoder.decode(returnType, config.resolve(value))); - } - } - - if (m.isDefault()) { + defaultSupplier = createAnnotatedMethodSupplier(m, returnType, config, decoder); + } else if (m.isDefault()) { defaultSupplier = createDefaultMethodSupplier(m, type, proxyObject); + } else { + defaultSupplier = knownCollections.getOrDefault(returnType, (ignored) -> null); } + final PropertyName nameAnnot = m.getAnnotation(PropertyName.class); final String propName = nameAnnot != null && nameAnnot.name() != null ? prefix + nameAnnot.name() @@ -303,54 +283,33 @@ else if ("toString".equals(method.getName())) { return proxyObject; } + private static Function createAnnotatedMethodSupplier(Method m, Class returnType, Config config, Decoder decoder) { + if (m.isDefault()) { + throw new IllegalArgumentException("@DefaultValue cannot be defined on a method with a default implementation for method " + + m.getDeclaringClass().getName() + "#" + m.getName()); + } else if ( + Map.class.isAssignableFrom(returnType) || + List.class.isAssignableFrom(returnType) || + Set.class.isAssignableFrom(returnType) ) { + throw new IllegalArgumentException("@DefaultValue cannot be used with collections. Use default method implemenation instead " + + m.getDeclaringClass().getName() + "#" + m.getName()); + } + + String value = m.getAnnotation(DefaultValue.class).value(); + if (returnType == String.class) { + return memoize((T) config.resolve(value)); + } else { + return memoize(decoder.decode(returnType, config.resolve(value))); + } + } + private static Function memoize(T value) { return (ignored) -> value; } - private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { - final MethodHandle methodHandle; - - try { - if (SystemUtils.IS_JAVA_1_8) { - Constructor constructor = MethodHandles.Lookup.class - .getDeclaredConstructor(Class.class, int.class); - constructor.setAccessible(true); - methodHandle = constructor.newInstance(type, MethodHandles.Lookup.PRIVATE) - .unreflectSpecial(method, type) - .bindTo(proxyObject); - } - else { - // Java 9 onwards - methodHandle = MethodHandles.lookup() - .findSpecial(type, - method.getName(), - MethodType.methodType(method.getReturnType(), method.getParameterTypes()), - type) - .bindTo(proxyObject); - } - } catch (ReflectiveOperationException e) { - throw new RuntimeException("Failed to create temporary object for " + type.getName(), e); - } - - return (args) -> { - try { - if (methodHandle.type().parameterCount() == 0) { - //noinspection unchecked - return (T) methodHandle.invokeWithArguments(); - } else if (args != null) { - //noinspection unchecked - return (T) methodHandle.invokeWithArguments(args); - } else { - // This is a handle to a method WITH arguments, being called with none. This happens when toString() - // is trying to build a representation of a proxy that has a parametrized property AND the interface - // provides a default method for it. There's no good default to return here, so we'll just use null - return null; - } - } catch (Throwable e) { - maybeWrapThenRethrow(e); - return null; // Unreachable, but the compiler doesn't know - } - }; + // type param ? is the return type for the METHOD, but we can't express that in Java + private static Function createDefaultMethodSupplier(Method method, Class type, T proxyObject) { + return DefaultMethodInvokerFactory.getFactory().findOrCreateDefaultMethodInvoker(type, method, proxyObject); } protected MethodInvoker createInterfaceProperty(String propName, final T proxy) { @@ -411,14 +370,4 @@ R getPropertyWithDefault(Class type, String propName) { } }; } - - private static void maybeWrapThenRethrow(Throwable t) { - if (t instanceof RuntimeException) { - throw (RuntimeException) t; - } - if (t instanceof Error) { - throw (Error) t; - } - throw new RuntimeException(t); - } } diff --git a/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java b/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java new file mode 100644 index 00000000..9e53d740 --- /dev/null +++ b/archaius2-core/src/main/java/com/netflix/archaius/DefaultMethodInvokerFactory.java @@ -0,0 +1,238 @@ +package com.netflix.archaius; + +import org.apache.commons.lang3.JavaVersion; +import org.apache.commons.lang3.SystemUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.invoke.CallSite; +import java.lang.invoke.LambdaMetafactory; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiFunction; +import java.util.function.Function; + +abstract class DefaultMethodInvokerFactory { + private static final Logger LOG = LoggerFactory.getLogger(DefaultMethodInvokerFactory.class); + + private static final DefaultMethodInvokerFactory INSTANCE; + static { + if (SystemUtils.isJavaVersionAtMost(JavaVersion.JAVA_1_8)) { + INSTANCE = new LegacyJava8Factory(); + } else if (SystemUtils.isJavaVersionAtMost(JavaVersion.JAVA_16)) { + INSTANCE = new BoundMethodHandlesFactory(); + } else { // Java 17 onwards + INSTANCE = new LambdasFactory(); + } + LOG.info("Choosing {} as invoker factory for default methods", INSTANCE.getClass()); + } + + static DefaultMethodInvokerFactory getFactory() { + return INSTANCE; + } + + /** + * Returns a Function object that acts as a call to invokerBound.method(...). + *

+ * {@literal <}R> is expected to be the METHOD's return type. Unfortunately this constraint can't be enforced by the compiler. + * + * @param methodOwner An interface that declares (or inherits) the wanted method. + * @param method The method to be called. This is expected to be a default method in METHOD_OWNER. + * @param invokerBound An instance of the METHOD_OWNER interface. + */ + abstract Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound); + + /** + * Shared "fall back" implementation using reflection to invoke the method handle. + */ + Function bindMethodHandle(MethodHandle methodHandle, T invokerBound) { + // Exceptions from this call are NOT caught because we want to fail fast if the arguments to the factory are bad. + MethodHandle boundHandle = methodHandle.bindTo(invokerBound); + + return (args) -> { + try { + if (boundHandle.type().parameterCount() == 0) { + + //noinspection unchecked + return (R) boundHandle.invokeWithArguments(); + } else if (args != null) { + + //noinspection unchecked + return (R) boundHandle.invokeWithArguments(args); + } else { + // This is a handle to a method WITH arguments, being called with none. This happens when + // invokerBound.toString() is called on an object that has a parametrized property AND the interface + // provides a default method for it. There's no good default to return here, so we'll just use null + return null; + } + } catch (Throwable e) { + maybeWrapThenRethrow(e); + return null; // Unreachable, but the compiler doesn't know + } + }; + + } + + + /** + * For Java 17 onwards, we wrap the method calls in lambdas and return that. This skips the reflection machinery and + * provides a noticeable performance boost. This implementation is known to fail in java 9 - 11. It *should* work + * from 12 or 13 onwards, but we have only tested it in 17 and 21. + */ + private static class LambdasFactory extends DefaultMethodInvokerFactory { + // We keep a running count of how many times we've seen a given Class. If we cross the threshold we stop creating + // lambdas for that class and fall back to the reflective implementation. This is motivated by an edge case we + // saw in clients, where they create (and leak) large numbers of proxies to the same interface. Because each lambda + // object requires its own anon class object, which lives in metaspace, and since users sometimes size their + // metaspaces to be much smaller than the heap, the leak becomes much more visible and causes OOMs. + private static final Map SEEN_COUNTS = new ConcurrentHashMap<>(); + + // The threshold is simply a best guess. Creating 1000 lambdas for the same interface is almost certainly + // a leak, right? + private static final int MAX_SEEN_THRESHOLD = 1000; + + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + MethodHandles.Lookup lookup = MethodHandles.lookup(); + MethodHandle methodHandle; + + try { + methodHandle = lookup.findSpecial( + method.getDeclaringClass(), + method.getName(), + MethodType.methodType(method.getReturnType(), method.getParameterTypes()), + method.getDeclaringClass()); + } catch (NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException(e); + } + + if (methodHandle.type().parameterCount() <= 2 && + SEEN_COUNTS.getOrDefault(methodOwner.getName(), 0) <= MAX_SEEN_THRESHOLD) { + + // For 1 or 2 argument handles (which translate to 0 and 1 argument *methods*), build a lambda object + // and use that to make the call. This avoids the cost of the reflection machinery and has a significant + // performance boost. + // But first, count how many times we've been here for this owner + SEEN_COUNTS.merge(methodOwner.getName(), 1, (oldValue, ignore) -> oldValue + 1); + + if (methodHandle.type().parameterCount() == 1) { + Function getter = asFunction(lookup, methodHandle); + //noinspection unchecked,DataFlowIssue + return (args) -> (R) getter.apply(invokerBound); + + } else if (methodHandle.type().parameterCount() == 2) { + BiFunction getter = asBiFunction(lookup, methodHandle); + //noinspection unchecked,DataFlowIssue + return (args) -> + args == null ? null : (R) getter.apply(invokerBound, args[0]); + } + } + + // Otherwise, for handles with more than 2 arguments or if we've reached the threshold, fall back to + // reflection + return bindMethodHandle(methodHandle, invokerBound); + } + + /** + * For a given no-args method M, return a (possibly cached) Function equivalent to the lambda + * instance -> instance.M(), + * where instance is an object of an adequate type. + */ + @SuppressWarnings("unchecked") + private static Function asFunction(MethodHandles.Lookup lookup, MethodHandle methodHandle) { + try { + CallSite site = LambdaMetafactory.metafactory(lookup, + "apply", + MethodType.methodType(Function.class), + MethodType.methodType(Object.class, Object.class), + methodHandle, + methodHandle.type()); + return (Function) site.getTarget().invokeExact(); + } catch (Throwable t) { + maybeWrapThenRethrow(t); + return null; // Unreachable, but the compiler doesn't know. + } + } + + @SuppressWarnings("unchecked") + private static BiFunction asBiFunction(MethodHandles.Lookup lookup, MethodHandle methodHandle) { + try { + CallSite site = LambdaMetafactory.metafactory(lookup, + "apply", + MethodType.methodType(BiFunction.class), + MethodType.methodType(Object.class, Object.class, Object.class), + methodHandle, + methodHandle.type()); + return (BiFunction) site.getTarget().invokeExact(); + } catch (Throwable t) { + maybeWrapThenRethrow(t); + return null; // Unreachable, but the compiler doesn't know + } + } + } + + /** + * An implementation safe to use in Java 9 - 16. It looks up a method handle which it then + * binds directly to the requested instance. + */ + private static class BoundMethodHandlesFactory extends DefaultMethodInvokerFactory { + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + final MethodHandle methodHandle; + + try { + methodHandle = MethodHandles.lookup() + .findSpecial(methodOwner, + method.getName(), + MethodType.methodType(method.getReturnType(), method.getParameterTypes()), + methodOwner); + } catch (NoSuchMethodException | IllegalAccessException e) { + throw new RuntimeException("Failed to create temporary object for " + methodOwner.getName(), e); + } + + return bindMethodHandle(methodHandle, invokerBound); + } + } + + /** + * For Java 8 we use a hacky mechanism (which got disabled in Java 9) to look up a method handle which we then + * bind directly to the requested function object. + */ + private static class LegacyJava8Factory extends DefaultMethodInvokerFactory { + + @Override + Function findOrCreateDefaultMethodInvoker(Class methodOwner, Method method, T invokerBound) { + final MethodHandle methodHandle; + + try { + Constructor constructor = MethodHandles.Lookup.class + .getDeclaredConstructor(Class.class, int.class); + constructor.setAccessible(true); + methodHandle = constructor.newInstance(methodOwner, MethodHandles.Lookup.PRIVATE) + .unreflectSpecial(method, methodOwner); + + } catch (NoSuchMethodException | InstantiationException | + IllegalAccessException | InvocationTargetException e) { + throw new RuntimeException("Failed to create temporary object for " + methodOwner.getName(), e); + } + + return bindMethodHandle(methodHandle, invokerBound); + } + } + + private static void maybeWrapThenRethrow(Throwable t) { + if (t instanceof RuntimeException) { + throw (RuntimeException) t; + } + if (t instanceof Error) { + throw (Error) t; + } + throw new RuntimeException(t); + } +} diff --git a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java index 56fbedde..310a19cc 100644 --- a/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java +++ b/archaius2-core/src/test/java/com/netflix/archaius/ProxyFactoryTest.java @@ -261,8 +261,13 @@ public void testWithArgumentsAndPrefix() { public interface WithArgumentsAndDefaultMethod { @PropertyName(name="${0}.abc.${1}") - default String getProperty(String part0, int part1) { - return "default"; + default String getPropertyWith2Placeholders(String part0, int part1) { + return "defaultFor2"; + } + + @PropertyName(name="${0}.def") + default String getPropertyWith1Placeholder(String part0) { + return "defaultFor1"; } } @@ -271,14 +276,18 @@ public void testWithArgumentsAndDefaultMethod() { SettableConfig config = new DefaultSettableConfig(); config.setProperty("a.abc.1", "value1"); config.setProperty("b.abc.2", "value2"); + config.setProperty("c.def", "value_c"); PropertyFactory factory = DefaultPropertyFactory.from(config); ConfigProxyFactory proxy = new ConfigProxyFactory(config, config.getDecoder(), factory); WithArgumentsAndDefaultMethod withArgsAndDefM = proxy.newProxy(WithArgumentsAndDefaultMethod.class); - Assert.assertEquals("value1", withArgsAndDefM.getProperty("a", 1)); - Assert.assertEquals("value2", withArgsAndDefM.getProperty("b", 2)); - Assert.assertEquals("default", withArgsAndDefM.getProperty("a", 2)); + Assert.assertEquals("value1", withArgsAndDefM.getPropertyWith2Placeholders("a", 1)); + Assert.assertEquals("value2", withArgsAndDefM.getPropertyWith2Placeholders("b", 2)); + Assert.assertEquals("defaultFor2", withArgsAndDefM.getPropertyWith2Placeholders("a", 2)); + + Assert.assertEquals("value_c", withArgsAndDefM.getPropertyWith1Placeholder("c")); + Assert.assertEquals("defaultFor1", withArgsAndDefM.getPropertyWith1Placeholder("q")); } public interface ConfigWithMaps { @@ -525,8 +534,10 @@ public void testObjectMethods_ClassWithArgumentsAndDefaultMethod() { // Printing 'null' here is a compromise. The default method in the interface is being called with a bad signature. // There's nothing the proxy could return here that isn't a lie, but at least this is a mostly harmless lie. - Assert.assertEquals("WithArgumentsAndDefaultMethod[${0}.abc.${1}='null']", withArgs.toString()); + Assert.assertEquals("WithArgumentsAndDefaultMethod[${0}.def='null',${0}.abc.${1}='null']", withArgs.toString()); + //noinspection ObviousNullCheck Assert.assertNotNull(withArgs.hashCode()); - Assert.assertTrue(withArgs.equals(withArgs)); + //noinspection EqualsWithItself + Assert.assertEquals(withArgs, withArgs); } } diff --git a/archaius2-persisted2/build.gradle b/archaius2-persisted2/build.gradle index c50f44c6..252f5e5a 100644 --- a/archaius2-persisted2/build.gradle +++ b/archaius2-persisted2/build.gradle @@ -21,7 +21,7 @@ dependencies { implementation 'com.fasterxml.jackson.core:jackson-core:2.4.3' implementation 'com.fasterxml.jackson.core:jackson-databind:2.4.3' implementation 'javax.annotation:javax.annotation-api:1.3.2' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' implementation 'org.slf4j:slf4j-api:1.7.36' testImplementation project(':archaius2-guice') diff --git a/archaius2-test/build.gradle b/archaius2-test/build.gradle index b8ba3bb9..84bf8d61 100644 --- a/archaius2-test/build.gradle +++ b/archaius2-test/build.gradle @@ -4,7 +4,7 @@ apply plugin: 'java-library' dependencies { api project(':archaius2-core') api 'junit:junit:4.13.2' - implementation 'org.apache.commons:commons-lang3:3.3.2' + implementation 'org.apache.commons:commons-lang3:3.12.0' } eclipse {