Skip to content

Commit

Permalink
Convert Netty wrapper cache to VirtualField
Browse files Browse the repository at this point in the history
  • Loading branch information
trask committed Nov 23, 2021
1 parent 2f57a21 commit dac1522
Showing 1 changed file with 25 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,19 @@
import io.netty.util.concurrent.ProgressiveFuture;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.cache.Cache;
import java.lang.ref.WeakReference;
import io.opentelemetry.instrumentation.api.field.VirtualField;

public final class FutureListenerWrappers {
// Instead of VirtualField use Cache with weak keys and weak values to store link between original
// listener and wrapper. VirtualField works fine when wrapper is stored in a field on original
// listener, but when listener class is a lambda instead of field it gets stored in a map with
// weak keys where original listener is key and wrapper is value. As wrapper has a strong
// reference to original listener this causes a memory leak.
// Also note that it's ok if the value is collected prior to the key, since this cache is only
// used to remove the wrapped listener from the netty future, and if the value is collected prior
// to the key, that means it's no longer used (referenced) by the netty future anyways.
private static final Cache<
GenericFutureListener<? extends Future<?>>,
WeakReference<GenericFutureListener<? extends Future<?>>>>
wrappers = Cache.weak();
// important: if this is ever converted to library instrumentation, this will create a memory leak
// because while library implementation of VirtualField maintains a weak reference to its keys, it
// maintains a strong reference to its values, and the wrapper has a strong reference to original
// listener, which will create a memory leak.
// this is not a problem in the javaagent's implementation of VirtualField, since it injects the
// value directly into the key as a field, and so the value is only retained strongly by the key,
// and so they can be collected together.
@SuppressWarnings("rawtypes")
private static final VirtualField<GenericFutureListener, GenericFutureListener> wrapperField =
VirtualField.find(GenericFutureListener.class, GenericFutureListener.class);

private static final ClassValue<Boolean> shouldWrap =
new ClassValue<Boolean>() {
Expand All @@ -46,32 +43,24 @@ public static boolean shouldWrap(GenericFutureListener<? extends Future<?>> list
@SuppressWarnings("unchecked")
public static GenericFutureListener<?> wrap(
Context context, GenericFutureListener<? extends Future<?>> delegate) {
WeakReference<GenericFutureListener<? extends Future<?>>> resultReference =
wrappers.computeIfAbsent(
delegate,
key -> {
GenericFutureListener<? extends Future<?>> wrapper;
if (delegate instanceof GenericProgressiveFutureListener) {
wrapper =
new WrappedProgressiveFutureListener(
context, (GenericProgressiveFutureListener<ProgressiveFuture<?>>) delegate);
} else {
wrapper =
new WrappedFutureListener(context, (GenericFutureListener<Future<?>>) delegate);
}
return new WeakReference<>(wrapper);
});
return resultReference.get();
GenericFutureListener<? extends Future<?>> wrapper = wrapperField.get(delegate);
if (wrapper == null) {
if (delegate instanceof GenericProgressiveFutureListener) {
wrapper =
new WrappedProgressiveFutureListener(
context, (GenericProgressiveFutureListener<ProgressiveFuture<?>>) delegate);
} else {
wrapper = new WrappedFutureListener(context, (GenericFutureListener<Future<?>>) delegate);
}
wrapperField.set(delegate, wrapper);
}
return wrapper;
}

@SuppressWarnings("unchecked")
public static GenericFutureListener<? extends Future<?>> getWrapper(
GenericFutureListener<? extends Future<?>> delegate) {
WeakReference<GenericFutureListener<? extends Future<?>>> wrapperReference =
wrappers.get(delegate);
if (wrapperReference == null) {
return delegate;
}
GenericFutureListener<? extends Future<?>> wrapper = wrapperReference.get();
GenericFutureListener<? extends Future<?>> wrapper = wrapperField.get(delegate);
return wrapper == null ? delegate : wrapper;
}

Expand Down

0 comments on commit dac1522

Please sign in to comment.