Skip to content

Commit

Permalink
Reworks core types so that their classes can be unloaded
Browse files Browse the repository at this point in the history
This involves more care with thread locals, and addition of classloader
tests.
  • Loading branch information
Adrian Cole authored and adriancole committed Sep 13, 2018
1 parent 005c574 commit 9d095d7
Show file tree
Hide file tree
Showing 38 changed files with 884 additions and 243 deletions.
Expand Up @@ -4,6 +4,7 @@
import brave.propagation.CurrentTraceContext;
import brave.propagation.CurrentTraceContext.Scope;
import brave.propagation.TraceContext;
import brave.test.util.ClassLoaders;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
Expand All @@ -14,20 +15,23 @@
import java.util.function.Supplier;
import org.junit.Test;

import static brave.test.util.ClassLoaders.assertRunIsUnloadableWithSupplier;
import static brave.test.util.ClassLoaders.newInstance;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;

public abstract class CurrentTraceContextTest {

protected abstract CurrentTraceContext newCurrentTraceContext();

protected final CurrentTraceContext currentTraceContext;
protected final TraceContext context =
TraceContext.newBuilder().traceIdHigh(-1L).traceId(1L).spanId(1L).build();
protected final TraceContext context2 =
context.toBuilder().parentId(context.spanId()).spanId(-2L).build();

protected abstract Class<? extends Supplier<CurrentTraceContext>> currentSupplier();

protected CurrentTraceContextTest() {
currentTraceContext = newCurrentTraceContext();
currentTraceContext = newInstance(currentSupplier(), getClass().getClassLoader()).get();
}

protected void verifyImplicitContext(@Nullable TraceContext context) {
Expand Down Expand Up @@ -141,12 +145,12 @@ protected void is_inheritable(CurrentTraceContext inheritableCurrentTraceContext

// submitting a job grows the pool, attaching the context to its thread
try (Scope scope = inheritableCurrentTraceContext.newScope(context)) {
assertThat(service.submit(() -> inheritableCurrentTraceContext.get()).get())
assertThat(service.submit(inheritableCurrentTraceContext::get).get())
.isEqualTo(context);
}

// same thread executes the next job and still has the same context (leaked and not cleaned up)
assertThat(service.submit(() -> inheritableCurrentTraceContext.get()).get())
assertThat(service.submit(inheritableCurrentTraceContext::get).get())
.isEqualTo(context);

service.shutdownNow();
Expand All @@ -165,7 +169,7 @@ protected void is_inheritable(CurrentTraceContext inheritableCurrentTraceContext
throw (Exception) e.getCause();
}

assertThat(service.submit(() -> currentTraceContext.get()).get())
assertThat(service.submit(currentTraceContext::get).get())
.isNull();
verifyImplicitContext(null);

Expand Down Expand Up @@ -244,4 +248,45 @@ protected void is_inheritable(CurrentTraceContext inheritableCurrentTraceContext
verifyImplicitContext(context0);
}
}

@Test public void unloadable_unused() {
assertRunIsUnloadableWithSupplier(Unused.class, currentSupplier());
}

static class Unused extends ClassLoaders.ConsumerRunnable<CurrentTraceContext> {
@Override public void accept(CurrentTraceContext currentTraceContext) {
}
}

@Test public void unloadable_afterScopeClose() {
assertRunIsUnloadableWithSupplier(ClosedScope.class, currentSupplier());
}

static class ClosedScope extends ClassLoaders.ConsumerRunnable<CurrentTraceContext> {
@Override public void accept(CurrentTraceContext current) {
try (Scope ws = current.newScope(TraceContext.newBuilder().traceId(1L).spanId(2L).build())) {
}
}
}

/**
* TODO: While it is an instrumentation bug to not close a scope, we should be tolerant. For
* example, considering weak references or similar.
*/
@SuppressWarnings("CheckReturnValue")
@Test public void notUnloadable_whenScopeLeaked() {
try {
assertRunIsUnloadableWithSupplier(LeakedScope.class, currentSupplier());
failBecauseExceptionWasNotThrown(AssertionError.class);
} catch (AssertionError e) {
// clear the leaked scope so other tests don't break
currentTraceContext.newScope(null);
}
}

static class LeakedScope extends ClassLoaders.ConsumerRunnable<CurrentTraceContext> {
@Override public void accept(CurrentTraceContext current) {
current.newScope(TraceContext.newBuilder().traceId(1L).spanId(2L).build());
}
}
}
Expand Up @@ -6,15 +6,19 @@
import brave.propagation.SamplingFlags;
import brave.propagation.TraceContext;
import brave.propagation.TraceContextOrSamplingFlags;
import brave.test.util.ClassLoaders;
import java.util.LinkedHashMap;
import java.util.Map;
import java.util.function.Supplier;
import org.junit.Test;

import static brave.test.util.ClassLoaders.assertRunIsUnloadableWithSupplier;
import static brave.test.util.ClassLoaders.newInstance;
import static org.assertj.core.api.Assertions.assertThat;

public abstract class PropagationTest<K> {

protected abstract Propagation<K> propagation();
protected abstract Class<? extends Supplier<Propagation<K>>> propagationSupplier();

protected abstract void inject(Map<K, String> map, @Nullable String traceId,
@Nullable String parentId, @Nullable String spanId, @Nullable Boolean sampled,
Expand All @@ -37,6 +41,12 @@ protected abstract void inject(Map<K, String> map, @Nullable String traceId,
.parentId(rootSpan.spanId())
.spanId(2).build();

protected final Propagation<K> propagation;

protected PropagationTest() {
propagation = newInstance(propagationSupplier(), getClass().getClassLoader()).get();
}

@Test public void verifyRoundTrip_rootSpan() throws Exception {
inject(map, "0000000000000001", null, "0000000000000001", true, null);

Expand Down Expand Up @@ -93,7 +103,8 @@ protected abstract void inject(Map<K, String> map, @Nullable String traceId,
}

/**
* When the caller propagates IDs, but not a sampling decision, the current process should decide.
* When the caller propagates IDs, but not a sampling decision, the current process should
* decide.
*/
@Test public void verifyRoundTrip_externallyProvidedIds() {
inject(map, "0000000000000001", null, "0000000000000001", null, null);
Expand All @@ -102,14 +113,14 @@ protected abstract void inject(Map<K, String> map, @Nullable String traceId,
}

void verifyRoundTrip(TraceContextOrSamplingFlags expected) {
TraceContextOrSamplingFlags extracted = propagation().extractor(mapEntry).extract(map);
TraceContextOrSamplingFlags extracted = propagation.extractor(mapEntry).extract(map);

assertThat(extracted)
.isEqualTo(expected);

Map<K, String> injected = new LinkedHashMap<>();
if (expected.context() != null) {
propagation().injector(mapEntry).inject(expected.context(), injected);
propagation.injector(mapEntry).inject(expected.context(), injected);
} else {
inject(injected, expected.samplingFlags());
}
Expand All @@ -120,7 +131,7 @@ void verifyRoundTrip(TraceContextOrSamplingFlags expected) {
protected static class MapEntry<K> implements
Propagation.Getter<Map<K, String>, K>,
Propagation.Setter<Map<K, String>, K> {
public MapEntry(){
public MapEntry() {
}

@Override public void put(Map<K, String> carrier, K key, String value) {
Expand All @@ -131,4 +142,31 @@ public MapEntry(){
return carrier.get(key);
}
}

@Test public void unloadable_unused() {
assertRunIsUnloadableWithSupplier(Unused.class, propagationSupplier());
}

static class Unused extends ClassLoaders.ConsumerRunnable<Propagation<?>> {
@Override public void accept(Propagation<?> propagation) {
}
}

@Test public void unloadable_afterBasicUsage() {
assertRunIsUnloadableWithSupplier(BasicUsage.class, propagationSupplier());
}

static class BasicUsage extends ClassLoaders.ConsumerRunnable<Propagation<?>> {
@Override public void accept(Propagation<?> propagation) {
TraceContext.Injector<Map<Object, String>> injector = propagation.injector(Map::put);
TraceContext.Extractor<Map<Object, String>> extractor = propagation.extractor(Map::get);

TraceContext ctx = TraceContext.newBuilder().traceId(1L).spanId(2L).sampled(false).build();
Map<Object, String> map = new LinkedHashMap<>();
injector.inject(ctx, map);

assertThat(extractor.extract(map).context())
.isEqualToComparingFieldByField(ctx);
}
}
}
145 changes: 145 additions & 0 deletions brave-tests/src/main/java/brave/test/util/ClassLoaders.java
@@ -0,0 +1,145 @@
package brave.test.util;

import java.io.File;
import java.lang.ref.WeakReference;
import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.function.Consumer;
import java.util.function.Supplier;

import static org.assertj.core.api.Assertions.assertThat;

public final class ClassLoaders {

/** Runs the assertion in a new classloader. Needed when you are creating parameterized tests */
public static <T> void assertRunIsUnloadableWithSupplier(
Class<? extends ConsumerRunnable<T>> assertion,
Class<? extends Supplier<? extends T>> supplier
) {
String property = assertion.getName() + ".supplier"; // assumes assertion is run only once
System.setProperty(property, supplier.getName());
try {
assertRunIsUnloadable(assertion, assertion.getClassLoader());
} finally {
System.getProperties().remove(property);
}
}

public static abstract class ConsumerRunnable<T> implements Runnable, Consumer<T> {
Class<? extends Supplier<T>> subjectSupplier;

protected ConsumerRunnable() {
try {
subjectSupplier =
(Class) Class.forName(System.getProperty(getClass().getName() + ".supplier"));
} catch (ClassNotFoundException e) {
throw new AssertionError(e);
}
}

@Override public void run() {
accept(newInstance(subjectSupplier, getClass().getClassLoader()).get());
}
}

/** Validating instance creator that ensures the supplier type is static or top-level */
public static <T> T newInstance(Class<T> type, ClassLoader loader) {
assertThat(type)
.withFailMessage(type + " should be a static member class")
.satisfies(c -> {
assertThat(c.isLocalClass()).isFalse();
assertThat(Modifier.isPublic(c.getModifiers())).isFalse();
});

try {
Class<T> classToInstantiate = (Class<T>) loader.loadClass(type.getName());
Constructor<T> ctor = classToInstantiate.getDeclaredConstructor();
ctor.setAccessible(true);

return ctor.newInstance();
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(e);
}
}

/** Runs the type in a new classloader that recreates brave classes */
public static void assertRunIsUnloadable(Class<? extends Runnable> runnable, ClassLoader parent) {
WeakReference<ClassLoader> loader;
try {
loader = invokeRunFromNewClassLoader(runnable, parent);
} catch (Exception e) {
throw new AssertionError(e);
}

blockOnGC();

assertThat(loader.get())
.withFailMessage(runnable + " includes state that couldn't be garbage collected")
.isNull();
}

static void blockOnGC() {
System.gc();
try {
Thread.sleep(200L);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new AssertionError(e);
}
}

static WeakReference<ClassLoader> invokeRunFromNewClassLoader(
Class<? extends Runnable> runnable, ClassLoader parent) throws Exception {

ClassLoader loader = ClassLoaders.reloadClassNamePrefix(parent, "brave");
Runnable instance = newInstance(runnable, loader);

// ensure the classes are indeed different
assertThat(instance.getClass()).isNotSameAs(runnable);

Method run = instance.getClass().getMethod("run");
run.setAccessible(true);

run.invoke(instance);

return new WeakReference<>(loader);
}

/**
* Creates a new classloader that reloads types matching the given prefix. This is used to test
* behavior such a leaked type in a thread local.
*
* <p>This works by using a bridge loader over the normal system one. The bridge loader always
* loads new classes when they are prefixed by brave types.
*
* <p>This approach is the same as what's used in Android's {@code tests.util.ClassLoaderBuilder}
* See https://android.googlesource.com/platform/libcore/+/master/support/src/test/java/tests/util/ClassLoaderBuilder.java
*/
static ClassLoader reloadClassNamePrefix(ClassLoader parent, String prefix) {
ClassLoader bridge = new ClassLoader(parent) {
@Override
protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundException {
if (name.startsWith(prefix)) throw new ClassNotFoundException("reloading type: " + name);
return super.loadClass(name, resolve);
}
};
try {
return new URLClassLoader(classpathToUrls(), bridge);
} catch (MalformedURLException e) {
throw new IllegalStateException("Couldn't read the current system classpath", e);
}
}

static URL[] classpathToUrls() throws MalformedURLException {
String[] classpath = System.getProperty("java.class.path").split(File.pathSeparator, -1);
URL[] result = new URL[classpath.length];
for (int i = 0; i < classpath.length; i++) {
result[i] = new File(classpath[i]).toURI().toURL();
}
return result;
}
}
19 changes: 19 additions & 0 deletions brave-tests/src/test/java/brave/TracerClassLoaderTest.java
@@ -0,0 +1,19 @@
package brave;

import org.junit.Test;
import zipkin2.Span;

import static brave.test.util.ClassLoaders.assertRunIsUnloadable;

public class TracerClassLoaderTest {
@Test public void unloadable_withLoggingReporter() {
assertRunIsUnloadable(UsingLoggingReporter.class, getClass().getClassLoader());
}

static class UsingLoggingReporter implements Runnable {
@Override public void run() {
Tracing.LoggingReporter reporter = new Tracing.LoggingReporter();
reporter.report(Span.newBuilder().traceId("a").id("b").build());
}
}
}

0 comments on commit 9d095d7

Please sign in to comment.