Add context getter/setter methods to an object only if not already defined
Class `FieldBackedProvider` uses ByteBuddy to add a field and its respective getters and setters to store the context object.
Assuming that we have a class `A` that implements runnable and that we wrap with a `FieldBackedProvider`
`Runnable` interfaces, if later on we use a cglib's `Enancher` class on `A` then the our mechanism will kick in again and try
to add the methods again. This causes a `java.lang.ClassFormatError: Duplicate method name "get__datadogContext$java$lang$Runnable" with signature "()Ljava.lang.Object;"`
to be thrown because CGLIB already copied over those methods from the original class `A` to the newly created class.
With this commit we now check that method were not previously defined before adding them, and if they were then we avoid adding them
again.
The reason why it wasn't faiing before is that we only checked on context field existence, not methods and cglib do not copy over fields, it copies only methods. E.g.
```
public class Main {
public static class A {
private String name = "hey";
public String getName() {
return this.name;
}
}
public static void main(String[] args) {
System.out.println("----- 'A' declared fields -----");
A s = new A();
for (Field f : s.getClass().getDeclaredFields()) {
System.out.println("field: " + f.getName());
}
for (Method m : s.getClass().getDeclaredMethods()) {
System.out.println("method: " + m.getName());
}
System.out.println("----- Proxy declared fields -----");
Enhancer enhancer = new Enhancer();
enhancer.setSuperclass(A.class);
enhancer.setCallback((FixedValue) () -> null);
A proxy = (A) enhancer.create();
for (Field f : proxy.getClass().getDeclaredFields()) {
System.out.println("field: " + f.getName());
}
for (Method m : proxy.getClass().getDeclaredMethods()) {
System.out.println("method: " + m.getName());
}
}
}
```
Results in:
```
----- 'A' declared fields -----
field: name
method: getName
----- Proxy declared fields -----
field: CGLIB$BOUND
field: CGLIB$FACTORY_DATA
field: CGLIB$THREAD_CALLBACKS
field: CGLIB$STATIC_CALLBACKS
field: CGLIB$CALLBACK_0
field: CGLIB$CALLBACK_FILTER
method: equals
method: toString
method: hashCode
method: clone
method: getName
method: newInstance
method: newInstance
method: newInstance
method: setCallbacks
method: CGLIB$SET_STATIC_CALLBACKS
method: CGLIB$SET_THREAD_CALLBACKS
method: getCallback
method: getCallbacks
method: CGLIB$STATICHOOK1
method: CGLIB$BIND_CALLBACKS
method: setCallback
```
This commit is contained in:
Luca Abbati
parent
3b3f6856e3
commit
3a6fedda14
|
|
@ -10,7 +10,7 @@ This project includes a `.editorconfig` file for basic editor settings. This fi
|
|||
Java files must be formatted using [google-java-format](https://github.com/google/google-java-format). Please run the following task to ensure files are formatted before committing:
|
||||
|
||||
```shell
|
||||
./gradlew :googleJavaFormat
|
||||
./gradlew googleJavaFormat
|
||||
```
|
||||
|
||||
Other source files (Groovy, Scala, etc) should ideally be formatted by Intellij Idea's default formatting, but are not enforced.
|
||||
|
|
|
|||
|
|
@ -428,9 +428,13 @@ public class FieldBackedProvider implements InstrumentationContextProvider {
|
|||
private final TypeDescription contextType =
|
||||
new TypeDescription.ForLoadedType(Object.class);
|
||||
private final String fieldName = getContextFieldName(keyClassName);
|
||||
private final String getterMethodName = getContextGetterName(keyClassName);
|
||||
private final String setterMethodName = getContextSetterName(keyClassName);
|
||||
private final TypeDescription interfaceType =
|
||||
getFieldAccessorInterface(keyClassName, contextClassName);
|
||||
private boolean foundField = false;
|
||||
private boolean foundGetter = false;
|
||||
private boolean foundSetter = false;
|
||||
|
||||
@Override
|
||||
public void visit(
|
||||
|
|
@ -462,12 +466,32 @@ public class FieldBackedProvider implements InstrumentationContextProvider {
|
|||
return super.visitField(access, name, descriptor, signature, value);
|
||||
}
|
||||
|
||||
@Override
|
||||
public MethodVisitor visitMethod(
|
||||
int access, String name, String descriptor, String signature, String[] exceptions) {
|
||||
if (name.equals(getterMethodName)) {
|
||||
foundGetter = true;
|
||||
}
|
||||
if (name.equals(setterMethodName)) {
|
||||
foundSetter = true;
|
||||
}
|
||||
return super.visitMethod(access, name, descriptor, signature, exceptions);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void visitEnd() {
|
||||
// Checking only for field existence is not enough as libraries like CGLIB only copy
|
||||
// public/protected methods and not fields (neither public nor private ones) when
|
||||
// the enhance a class.
|
||||
// For this reason we check separately for the field and for the two accessors.
|
||||
if (!foundField) {
|
||||
cv.visitField(
|
||||
Opcodes.ACC_PRIVATE, fieldName, contextType.getDescriptor(), null, null);
|
||||
}
|
||||
if (!foundGetter) {
|
||||
addGetter();
|
||||
}
|
||||
if (!foundSetter) {
|
||||
addSetter();
|
||||
}
|
||||
super.visitEnd();
|
||||
|
|
@ -475,7 +499,7 @@ public class FieldBackedProvider implements InstrumentationContextProvider {
|
|||
|
||||
/** Just 'standard' getter implementation */
|
||||
private void addGetter() {
|
||||
final MethodVisitor mv = getAccessorMethodVisitor(getContextGetterName(keyClassName));
|
||||
final MethodVisitor mv = getAccessorMethodVisitor(getterMethodName);
|
||||
mv.visitCode();
|
||||
mv.visitVarInsn(Opcodes.ALOAD, 0);
|
||||
mv.visitFieldInsn(
|
||||
|
|
@ -490,7 +514,7 @@ public class FieldBackedProvider implements InstrumentationContextProvider {
|
|||
|
||||
/** Just 'standard' setter implementation */
|
||||
private void addSetter() {
|
||||
final MethodVisitor mv = getAccessorMethodVisitor(getContextSetterName(keyClassName));
|
||||
final MethodVisitor mv = getAccessorMethodVisitor(setterMethodName);
|
||||
mv.visitCode();
|
||||
mv.visitVarInsn(Opcodes.ALOAD, 0);
|
||||
mv.visitVarInsn(Opcodes.ALOAD, 1);
|
||||
|
|
|
|||
|
|
@ -1,16 +1,21 @@
|
|||
package context
|
||||
|
||||
|
||||
import datadog.trace.agent.test.AgentTestRunner
|
||||
import datadog.trace.agent.test.utils.ClasspathUtils
|
||||
import datadog.trace.api.Config
|
||||
import datadog.trace.util.gc.GCUtils
|
||||
import net.bytebuddy.agent.ByteBuddyAgent
|
||||
import net.bytebuddy.utility.JavaModule
|
||||
import net.sf.cglib.proxy.Enhancer
|
||||
import net.sf.cglib.proxy.MethodInterceptor
|
||||
import net.sf.cglib.proxy.MethodProxy
|
||||
import spock.lang.Requires
|
||||
|
||||
import java.lang.instrument.ClassDefinition
|
||||
import java.lang.ref.WeakReference
|
||||
import java.lang.reflect.Field
|
||||
import java.lang.reflect.Method
|
||||
import java.util.concurrent.atomic.AtomicReference
|
||||
|
||||
import static context.ContextTestInstrumentation.IncorrectCallUsageKeyClass
|
||||
|
|
@ -97,6 +102,51 @@ class FieldBackedProviderTest extends AgentTestRunner {
|
|||
new UntransformableKeyClass() | _
|
||||
}
|
||||
|
||||
static class ClassWithContextGetter extends KeyClass {
|
||||
def 'get__datadogContext$context$ContextTestInstrumentation$KeyClass'() {
|
||||
return new Object()
|
||||
}
|
||||
}
|
||||
|
||||
static class ClassWithContextSetter extends KeyClass {
|
||||
void 'set__datadogContext$context$ContextTestInstrumentation$KeyClass'(Object value) {}
|
||||
}
|
||||
|
||||
def "works with classes already having a the context getter method defined"() {
|
||||
when:
|
||||
new ClassWithContextGetter()
|
||||
|
||||
then:
|
||||
noExceptionThrown()
|
||||
}
|
||||
|
||||
def "works with classes already having the context setter method defined"() {
|
||||
when:
|
||||
new ClassWithContextSetter()
|
||||
|
||||
then:
|
||||
noExceptionThrown()
|
||||
}
|
||||
|
||||
def "works with cglib enhanced instances which duplicates context getter and setter methods"() {
|
||||
setup:
|
||||
Enhancer enhancer = new Enhancer()
|
||||
enhancer.setSuperclass(KeyClass.class)
|
||||
enhancer.setCallback(new MethodInterceptor() {
|
||||
@Override
|
||||
Object intercept(Object arg0, Method arg1, Object[] arg2,
|
||||
MethodProxy arg3) throws Throwable {
|
||||
return arg3.invokeSuper(arg0, arg2)
|
||||
}
|
||||
})
|
||||
|
||||
when:
|
||||
(KeyClass) enhancer.create()
|
||||
|
||||
then:
|
||||
noExceptionThrown()
|
||||
}
|
||||
|
||||
def "backing map should not create strong refs to key class instances #keyValue.get().getClass().getName()"() {
|
||||
when:
|
||||
final int count = keyValue.get().incrementContextCount()
|
||||
|
|
|
|||
|
|
@ -34,6 +34,7 @@ dependencies {
|
|||
testCompile project(':utils:gc-utils')
|
||||
|
||||
// test instrumenting java 1.1 bytecode
|
||||
testCompile group: 'cglib', name: 'cglib', version: '3.2.5'
|
||||
testCompile group: 'net.sf.jt400', name: 'jt400', version: '6.1'
|
||||
|
||||
// We have autoservices defined in test subtree, looks like we need this to be able to properly rebuild this
|
||||
|
|
|
|||
Loading…
Reference in New Issue