-
Notifications
You must be signed in to change notification settings - Fork 355
Return empty method lines instead of throwing when class bytecode resource is missing #12380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| package datadog.trace.civisibility.source; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import datadog.trace.civisibility.source.LinesResolver.Lines; | ||
| import java.io.ByteArrayOutputStream; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.lang.reflect.Method; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class ByteCodeLinesResolverTest { | ||
|
|
||
| @Test | ||
| void testMethodLinesResolution() throws NoSuchMethodException { | ||
| Method aTestMethod = NestedClass.class.getDeclaredMethod("aTestMethod"); | ||
|
|
||
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines methodLines = linesResolver.getMethodLines(aTestMethod); | ||
|
|
||
| assertTrue(methodLines.isValid()); | ||
| assertTrue(methodLines.getStartLineNumber() > 0); | ||
| assertTrue(methodLines.getEndLineNumber() > methodLines.getStartLineNumber()); | ||
| } | ||
|
|
||
| @Test | ||
| void testAlwaysInvalidClassLinesResolution() { | ||
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines classLines = linesResolver.getClassLines(NestedClass.class); | ||
|
|
||
| assertFalse(classLines.isValid()); | ||
| } | ||
|
|
||
| @Test | ||
| void testInvalidMethodLinesResolution() throws NoSuchMethodException { | ||
| Method abstractMethod = NestedClass.class.getDeclaredMethod("abstractMethod"); | ||
|
|
||
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines methodLines = linesResolver.getMethodLines(abstractMethod); | ||
|
|
||
| assertFalse(methodLines.isValid()); | ||
| } | ||
|
|
||
| @Test | ||
| void testReturnsEmptyMethodLinesWhenClassCannotBeLoaded() | ||
| throws IOException, ClassNotFoundException, NoSuchMethodException { | ||
| MisbehavingClassLoader misbehavingClassLoader = new MisbehavingClassLoader(); | ||
|
|
||
| try (InputStream stream = Utils.getClassStream(NestedClass.class)) { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = stream.read(buffer)) != -1) { | ||
| baos.write(buffer, 0, bytesRead); | ||
| } | ||
| misbehavingClassLoader.putClass(NestedClass.class.getName(), baos.toByteArray()); | ||
| } | ||
|
|
||
| Class<?> misbehavingClass = misbehavingClassLoader.loadClass(NestedClass.class.getName()); | ||
| Method misbehavingMethod = misbehavingClass.getDeclaredMethod("aTestMethod"); | ||
|
|
||
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines methodLines = linesResolver.getMethodLines(misbehavingMethod); | ||
|
|
||
| assertFalse(methodLines.isValid()); | ||
| } | ||
|
|
||
| @Test | ||
| void testReturnsEmptyMethodLinesWhenClassResourceIsMissing() | ||
| throws IOException, ClassNotFoundException, NoSuchMethodException { | ||
| // regression test: Utils.getClassStream() returns null (rather than throwing) for | ||
| // classes whose bytecode resource cannot be located (e.g. certain generated/proxy classes) | ||
| NullResourceClassLoader nullResourceClassLoader = new NullResourceClassLoader(); | ||
|
|
||
| try (InputStream stream = Utils.getClassStream(NestedClass.class)) { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| byte[] buffer = new byte[1024]; | ||
| int bytesRead; | ||
| while ((bytesRead = stream.read(buffer)) != -1) { | ||
| baos.write(buffer, 0, bytesRead); | ||
| } | ||
| nullResourceClassLoader.putClass(NestedClass.class.getName(), baos.toByteArray()); | ||
| } | ||
|
|
||
| Class<?> unresolvableClass = nullResourceClassLoader.loadClass(NestedClass.class.getName()); | ||
| Method unresolvableMethod = unresolvableClass.getDeclaredMethod("aTestMethod"); | ||
|
|
||
| ByteCodeLinesResolver linesResolver = new ByteCodeLinesResolver(); | ||
| Lines methodLines = linesResolver.getMethodLines(unresolvableMethod); | ||
|
|
||
| assertFalse(methodLines.isValid()); | ||
|
Comment on lines
+89
to
+92
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This assertion also passes against the previous implementation: Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| @Test | ||
| void testReturnsEmptyMethodLinesWhenUnknownMethodIsAttemptedToBeResolved() | ||
| throws NoSuchMethodException { | ||
| Method abstractMethod = NestedClass.class.getDeclaredMethod("abstractMethod"); | ||
| ByteCodeLinesResolver.ClassMethodLines classMethodLines = | ||
| new ByteCodeLinesResolver.ClassMethodLines(); | ||
|
|
||
| Lines methodLines = classMethodLines.get(abstractMethod); | ||
|
|
||
| assertFalse(methodLines.isValid()); | ||
| } | ||
|
|
||
| private abstract static class NestedClass { | ||
| static double aTestMethod() { | ||
| double random = Math.random(); | ||
| return random; | ||
| } | ||
|
|
||
| abstract void abstractMethod(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||||||||||||||||||||||||
| package datadog.trace.civisibility.source; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import java.io.InputStream; | ||||||||||||||||||||||||||||
| import java.util.HashMap; | ||||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||
| * A {@link ClassLoader} that defines classes from an in-memory map and returns {@code null} from | ||||||||||||||||||||||||||||
| * resource lookups (rather than throwing), used to exercise {@code ByteCodeLinesResolver} when a | ||||||||||||||||||||||||||||
| * class's bytecode resource cannot be located. | ||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||
| * <p>Kept in Java rather than the Java 8 test suite's Groovy counterpart on purpose: see {@link | ||||||||||||||||||||||||||||
| * MisbehavingClassLoader} for why. | ||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||
|
Comment on lines
+7
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||||||||||||||||||||||||
| final class NullResourceClassLoader extends ClassLoader { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| private final Map<String, byte[]> classes = new HashMap<>(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
| public InputStream getResourceAsStream(String name) { | ||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||||
| public Class<?> loadClass(String name) throws ClassNotFoundException { | ||||||||||||||||||||||||||||
| byte[] bytes = classes.get(name); | ||||||||||||||||||||||||||||
| if (bytes != null) { | ||||||||||||||||||||||||||||
| return defineClass(name, bytes, 0, bytes.length); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| return super.loadClass(name); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| void putClass(String name, byte[] bytes) { | ||||||||||||||||||||||||||||
| classes.put(name, bytes); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a custom class loader temporarily returns
nullbut can provide the resource later, returning this non-null empty value fromClassMethodLines.parsecausesmethodLinesCache.computeIfAbsentto cache the failed lookup. Subsequent calls that hit the entry therefore never retry and continue returning missing method borders, unlike the existing exception path explicitly marked “do not cache failure”; handle the null case without storing a negative result while still suppressing the error-level log.Useful? React with 👍 / 👎.