Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions api/src/main/java/io/grpc/InternalServiceProviders.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.grpc;

import com.google.common.annotations.VisibleForTesting;
import java.util.Iterator;
import java.util.List;

@Internal
Expand All @@ -27,23 +28,24 @@ private InternalServiceProviders() {
/**
* Accessor for method.
*/
public static <T> T load(
//@Deprecated
public static <T> List<T> loadAll(
Class<T> klass,
Iterable<Class<?>> hardcoded,
Iterable<Class<?>> hardCodedClasses,
ClassLoader classLoader,
PriorityAccessor<T> priorityAccessor) {
return ServiceProviders.load(klass, hardcoded, classLoader, priorityAccessor);
return ServiceProviders.loadAll(klass, hardCodedClasses, classLoader, priorityAccessor);
}

/**
* Accessor for method.
*/
public static <T> List<T> loadAll(
Class<T> klass,
Iterator<T> serviceLoader,
Iterable<Class<?>> hardCodedClasses,
ClassLoader classLoader,
PriorityAccessor<T> priorityAccessor) {
return ServiceProviders.loadAll(klass, hardCodedClasses, classLoader, priorityAccessor);
return ServiceProviders.loadAll(klass, serviceLoader, hardCodedClasses, priorityAccessor);
}

/**
Expand Down
5 changes: 4 additions & 1 deletion api/src/main/java/io/grpc/NameResolverRegistry.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.ServiceLoader;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -125,8 +126,10 @@ public static synchronized NameResolverRegistry getDefaultRegistry() {
if (instance == null) {
List<NameResolverProvider> providerList = ServiceProviders.loadAll(
NameResolverProvider.class,
ServiceLoader
.load(NameResolverProvider.class, NameResolverProvider.class.getClassLoader())
.iterator(),
getHardCodedClasses(),
NameResolverProvider.class.getClassLoader(),
new NameResolverPriorityAccessor());
if (providerList.isEmpty()) {
logger.warning("No NameResolverProviders found via ServiceLoader, including for DNS. This "
Expand Down
56 changes: 37 additions & 19 deletions api/src/main/java/io/grpc/ServiceProviders.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.ListIterator;
import java.util.ServiceConfigurationError;
import java.util.ServiceLoader;

Expand All @@ -30,41 +32,58 @@ private ServiceProviders() {
}

/**
* If this is not Android, returns the highest priority implementation of the class via
* If this is not Android, returns all available implementations discovered via
* {@link ServiceLoader}.
* If this is Android, returns an instance of the highest priority class in {@code hardcoded}.
* If this is Android, returns all available implementations in {@code hardcoded}.
* The list is sorted in descending priority order.
*/
public static <T> T load(
//@Deprecated
public static <T> List<T> loadAll(
Class<T> klass,
Iterable<Class<?>> hardcoded,
ClassLoader cl,
PriorityAccessor<T> priorityAccessor) {
List<T> candidates = loadAll(klass, hardcoded, cl, priorityAccessor);
if (candidates.isEmpty()) {
return null;
}
return candidates.get(0);
final PriorityAccessor<T> priorityAccessor) {
return loadAll(klass, ServiceLoader.load(klass, cl).iterator(), hardcoded, priorityAccessor);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're concerned about the performance of clients not using R8, I think you'll want to make the call to ServiceLoader.load() conditional on R8 being used. Otherwise the hardcoded fallback will never be used. I'm not aware of a way to do that aside from adding an -assumevalues rule.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my reading of ServiceLoader, load() doesn't do any reflection. All the reflection is done when iterating. So while we may create the ServiceLoader here, we'll throw the ServiceLoader's iterator away without using it on Android if it isn't derived from a List. That's a bit of a hack, but also seems reasonably likely to remain stable. But -assumevalues might be a good option too (assuming we can set it without upsetting non-R8 shrinking tools).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed! Does look that way on android at least.

}

/**
* If this is not Android, returns all available implementations discovered via
* {@link ServiceLoader}.
* If this is Android, returns all available implementations in {@code hardcoded}.
* The list is sorted in descending priority order.
*
* <p>{@code serviceLoader} should be created with {@code ServiceLoader.load(MyClass.class,
* MyClass.class.getClassLoader()).iterator()} in order to be detected by R8 so that R8 full mode
* will keep the constructors for the provider classes.
*/
public static <T> List<T> loadAll(
Class<T> klass,
Iterator<T> serviceLoader,
Iterable<Class<?>> hardcoded,
ClassLoader cl,
final PriorityAccessor<T> priorityAccessor) {
Iterable<T> candidates;
if (isAndroid(cl)) {
candidates = getCandidatesViaHardCoded(klass, hardcoded);
Iterator<T> candidates;
if (serviceLoader instanceof ListIterator) {
// A rewriting tool has replaced the ServiceLoader with a List of some sort (R8 uses
// ArrayList, AppReduce uses singletonList). We prefer to use such iterators on Android as
// they won't need reflection like the hard-coded list does. In addition, the provider
// instances will have already been created, so it seems we should use them.
//
// R8: https://r8.googlesource.com/r8/+/490bc53d9310d4cc2a5084c05df4aadaec8c885d/src/main/java/com/android/tools/r8/ir/optimize/ServiceLoaderRewriter.java
// AppReduce: service_loader_pass.cc
candidates = serviceLoader;
} else if (isAndroid(klass.getClassLoader())) {
// Avoid getResource() on Android, which must read from a zip which uses a lot of memory
candidates = getCandidatesViaHardCoded(klass, hardcoded).iterator();
} else if (!serviceLoader.hasNext()) {
// Attempt to load using the context class loader and ServiceLoader.
// This allows frameworks like http://aries.apache.org/modules/spi-fly.html to plug in.
candidates = ServiceLoader.load(klass).iterator();
} else {
candidates = getCandidatesViaServiceLoader(klass, cl);
candidates = serviceLoader;
}
List<T> list = new ArrayList<>();
for (T current: candidates) {
while (candidates.hasNext()) {
T current = candidates.next();
if (!priorityAccessor.isAvailable(current)) {
continue;
}
Expand Down Expand Up @@ -101,15 +120,14 @@ static boolean isAndroid(ClassLoader cl) {
}

/**
* Loads service providers for the {@code klass} service using {@link ServiceLoader}.
* For testing only: Loads service providers for the {@code klass} service using {@link
* ServiceLoader}. Does not support spi-fly and related tricks.
*/
@VisibleForTesting
public static <T> Iterable<T> getCandidatesViaServiceLoader(Class<T> klass, ClassLoader cl) {
Iterable<T> i = ServiceLoader.load(klass, cl);
// Attempt to load using the context class loader and ServiceLoader.
// This allows frameworks like http://aries.apache.org/modules/spi-fly.html to plug in.
if (!i.iterator().hasNext()) {
i = ServiceLoader.load(klass);
return null;
}
return i;
}
Expand Down
32 changes: 20 additions & 12 deletions api/src/test/java/io/grpc/ServiceProvidersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ public void contextClassLoaderProvider() {
Thread.currentThread().setContextClassLoader(rcll);
assertEquals(
Available7Provider.class,
ServiceProviders.load(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());
load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());
} finally {
Thread.currentThread().setContextClassLoader(ccl);
}
Expand All @@ -85,8 +84,7 @@ public void noProvider() {
serviceFile,
"io/grpc/ServiceProvidersTestAbstractProvider-doesNotExist.txt");
Thread.currentThread().setContextClassLoader(cl);
assertNull(ServiceProviders.load(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR));
assertNull(load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR));
} finally {
Thread.currentThread().setContextClassLoader(ccl);
}
Expand All @@ -98,8 +96,7 @@ public void multipleProvider() throws Exception {
"io/grpc/ServiceProvidersTestAbstractProvider-multipleProvider.txt");
assertSame(
Available7Provider.class,
ServiceProviders.load(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());
load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());

List<ServiceProvidersTestAbstractProvider> providers = ServiceProviders.loadAll(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR);
Expand All @@ -116,16 +113,15 @@ public void unavailableProvider() {
"io/grpc/ServiceProvidersTestAbstractProvider-unavailableProvider.txt");
assertEquals(
Available7Provider.class,
ServiceProviders.load(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());
load(ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR).getClass());
}

@Test
public void unknownClassProvider() {
ClassLoader cl = new ReplacingClassLoader(getClass().getClassLoader(), serviceFile,
"io/grpc/ServiceProvidersTestAbstractProvider-unknownClassProvider.txt");
try {
ServiceProviders.load(
ServiceProviders.loadAll(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR);
fail("Exception expected");
} catch (ServiceConfigurationError e) {
Expand All @@ -140,7 +136,7 @@ public void exceptionSurfacedToCaller_failAtInit() {
try {
// Even though there is a working provider, if any providers fail then we should fail
// completely to avoid returning something unexpected.
ServiceProviders.load(
ServiceProviders.loadAll(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR);
fail("Expected exception");
} catch (ServiceConfigurationError expected) {
Expand All @@ -154,7 +150,7 @@ public void exceptionSurfacedToCaller_failAtPriority() {
"io/grpc/ServiceProvidersTestAbstractProvider-failAtPriorityProvider.txt");
try {
// The exception should be surfaced to the caller
ServiceProviders.load(
ServiceProviders.loadAll(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR);
fail("Expected exception");
} catch (FailAtPriorityProvider.PriorityException expected) {
Expand All @@ -168,7 +164,7 @@ public void exceptionSurfacedToCaller_failAtAvailable() {
"io/grpc/ServiceProvidersTestAbstractProvider-failAtAvailableProvider.txt");
try {
// The exception should be surfaced to the caller
ServiceProviders.load(
ServiceProviders.loadAll(
ServiceProvidersTestAbstractProvider.class, NO_HARDCODED, cl, ACCESSOR);
fail("Expected exception");
} catch (FailAtAvailableProvider.AvailableException expected) {
Expand Down Expand Up @@ -244,6 +240,18 @@ class RandomClass {}
assertFalse(candidates.iterator().hasNext());
}

private static <T> T load(
Class<T> klass,
Iterable<Class<?>> hardcoded,
ClassLoader cl,
PriorityAccessor<T> priorityAccessor) {
List<T> candidates = ServiceProviders.loadAll(klass, hardcoded, cl, priorityAccessor);
if (candidates.isEmpty()) {
return null;
}
return candidates.get(0);
}

private static class BaseProvider extends ServiceProvidersTestAbstractProvider {
private final boolean isAvailable;
private final int priority;
Expand Down
Loading