From fb5762923459b11ddcbc2b2b7d890af36aa9c5d9 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 30 Dec 2020 17:40:20 +0200 Subject: [PATCH 1/4] Close JarFile --- .../javaagent/tooling/ExporterClassLoader.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExporterClassLoader.java b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExporterClassLoader.java index cdfd3802b24e..b373636793a4 100644 --- a/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExporterClassLoader.java +++ b/javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExporterClassLoader.java @@ -9,6 +9,7 @@ import java.io.IOException; import java.io.InputStream; +import java.net.URISyntaxException; import java.net.URL; import java.net.URLClassLoader; import java.util.Enumeration; @@ -127,10 +128,9 @@ private static String getPackageName(String className) { } private static Manifest getManifest(URL url) { - try { - JarFile jarFile = new JarFile(url.getFile()); + try (JarFile jarFile = new JarFile(url.toURI().getPath())) { return jarFile.getManifest(); - } catch (IOException e) { + } catch (IOException | URISyntaxException e) { log.warn(e.getMessage(), e); } return null; From c1c4b2268f6f35278e05c8e3273e4b7093b15bdd Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 4 Jan 2021 19:45:19 +0200 Subject: [PATCH 2/4] test loading exporter jar that has a space in the path --- .../tooling/ExporterClassLoaderTest.groovy | 107 +++++++++++++++++- 1 file changed, 103 insertions(+), 4 deletions(-) diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy index 9c6d2c6461d4..c298d6bc7b2e 100644 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy @@ -10,8 +10,11 @@ import io.opentelemetry.javaagent.spi.exporter.SpanExporterFactory import io.opentelemetry.sdk.metrics.export.MetricExporter import io.opentelemetry.sdk.trace.export.SpanExporter import java.nio.charset.StandardCharsets +import java.util.jar.Attributes import java.util.jar.JarEntry +import java.util.jar.JarFile import java.util.jar.JarOutputStream +import java.util.jar.Manifest import spock.lang.Specification class ExporterClassLoaderTest extends Specification { @@ -19,7 +22,7 @@ class ExporterClassLoaderTest extends Specification { // Verifies https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/542 def "does not look in parent classloader for metric exporters"() { setup: - def parentClassloader = new URLClassLoader([createJarWithClasses(MetricExporterFactoryParent)] as URL[]) + def parentClassloader = new ParentClassLoader([createJarWithClasses(MetricExporterFactoryParent)] as URL[]) def childClassloader = new ExporterClassLoader(createJarWithClasses(MetricExporterFactoryChild), parentClassloader) when: @@ -27,11 +30,21 @@ class ExporterClassLoaderTest extends Specification { then: serviceLoader.size() == 1 + + and: + childClassloader.manifest != null + + when: + MetricExporterFactory instance = serviceLoader.iterator().next() + Class clazz = instance.getClass() + + then: + clazz.getClassLoader() == childClassloader } def "does not look in parent classloader for span exporters"() { setup: - def parentClassloader = new URLClassLoader([createJarWithClasses(SpanExporterFactoryParent)] as URL[]) + def parentClassloader = new ParentClassLoader([createJarWithClasses(SpanExporterFactoryParent)] as URL[]) def childClassloader = new ExporterClassLoader(createJarWithClasses(SpanExporterFactoryChild), parentClassloader) when: @@ -39,6 +52,43 @@ class ExporterClassLoaderTest extends Specification { then: serviceLoader.size() == 1 + + and: + childClassloader.manifest != null + + when: + SpanExporterFactory instance = serviceLoader.iterator().next() + Class clazz = instance.getClass() + + then: + clazz.getClassLoader() == childClassloader + } + + // Verifies that loading of exporter jar succeeds when there is a space in path to exporter jar + def "load jar with space in path"() { + setup: + def parentClassloader = new ParentClassLoader() + // " .jar" is used to make path to jar contain a space + def childClassloader = new ExporterClassLoader(createJarWithClasses(" .jar", MetricExporterFactoryChild), parentClassloader) + + when: + ServiceLoader serviceLoader = ServiceLoader.load(MetricExporterFactory, childClassloader) + + then: + serviceLoader.size() == 1 + + and: + childClassloader.manifest != null + + when: + MetricExporterFactory instance = serviceLoader.iterator().next() + Class clazz = instance.getClass() + + then: + clazz.getClassLoader() == childClassloader + + and: + clazz.getPackage().getImplementationVersion() == "test-implementation-version" } static class MetricExporterFactoryParent implements MetricExporterFactory { @@ -93,15 +143,35 @@ class ExporterClassLoaderTest extends Specification { } } - static URL createJarWithClasses(final Class... classes) + static URL createJarWithClasses(final Class... classes) { + createJarWithClasses(".jar", classes) + } + + static URL createJarWithClasses(final String suffix, final Class... classes) throws IOException { - File tmpJar = File.createTempFile(UUID.randomUUID().toString() + "-", ".jar") + File tmpJar = File.createTempFile(UUID.randomUUID().toString() + "-", suffix) tmpJar.deleteOnExit() JarOutputStream target = new JarOutputStream(new FileOutputStream(tmpJar)) for (Class clazz : classes) { addToJar(clazz, clazz.getInterfaces()[0], target) } + + Manifest manifest = new Manifest() + Attributes attributes = manifest.getMainAttributes() + attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0") + attributes.put(Attributes.Name.SPECIFICATION_TITLE, "test-specification-title") + attributes.put(Attributes.Name.SPECIFICATION_VERSION, "test-specification-version") + attributes.put(Attributes.Name.SPECIFICATION_VENDOR, "test-specification-vendor") + attributes.put(Attributes.Name.IMPLEMENTATION_TITLE, "test-implementation-title") + attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, "test-implementation-version") + attributes.put(Attributes.Name.IMPLEMENTATION_VENDOR, "test-implementation-vendor") + + JarEntry manifestEntry = new JarEntry(JarFile.MANIFEST_NAME) + target.putNextEntry(manifestEntry) + manifest.write(target) + target.closeEntry() + target.close() return tmpJar.toURI().toURL() @@ -149,4 +219,33 @@ class ExporterClassLoaderTest extends Specification { private static String getResourceName(final String className) { return className.replace('.', '/') + ".class" } + + static class ParentClassLoader extends URLClassLoader { + + ParentClassLoader() { + super() + } + + ParentClassLoader(URL[] urls) { + super(urls) + } + + @Override + Package getPackage(String name) { + // ExporterClassLoader uses getPackage to check whether package has already been + // defined. As getPackage also searches packages from parent class loader we return + // null here to ensure that package is defined in ExporterClassLoader. + null + } + + @Override + Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + // test classes are available in system class loader filter them so that + // they would be loaded by ExporterClassLoader + if (name.startsWith(ExporterClassLoaderTest.getName())) { + throw new ClassNotFoundException(name) + } + return super.loadClass(name, resolve) + } + } } From 4f2251b733d58f91b5832e06d755bf03606040ad Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 4 Jan 2021 20:21:03 +0200 Subject: [PATCH 3/4] change class to private --- .../javaagent/tooling/ExporterClassLoaderTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy index c298d6bc7b2e..6f47699ceae4 100644 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy @@ -220,7 +220,7 @@ class ExporterClassLoaderTest extends Specification { return className.replace('.', '/') + ".class" } - static class ParentClassLoader extends URLClassLoader { + private static class ParentClassLoader extends URLClassLoader { ParentClassLoader() { super() From a58a2aae9eb0677e0595b40c3c0b094d33270788 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Mon, 4 Jan 2021 21:25:09 +0200 Subject: [PATCH 4/4] try to convince groovy compiler not to create references to java.lang.Module --- .../javaagent/tooling/ExporterClassLoaderTest.groovy | 2 ++ 1 file changed, 2 insertions(+) diff --git a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy index 6f47699ceae4..4866ae899c84 100644 --- a/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy +++ b/javaagent-tooling/src/test/groovy/io/opentelemetry/javaagent/tooling/ExporterClassLoaderTest.groovy @@ -5,6 +5,7 @@ package io.opentelemetry.javaagent.tooling +import groovy.transform.CompileStatic import io.opentelemetry.javaagent.spi.exporter.MetricExporterFactory import io.opentelemetry.javaagent.spi.exporter.SpanExporterFactory import io.opentelemetry.sdk.metrics.export.MetricExporter @@ -220,6 +221,7 @@ class ExporterClassLoaderTest extends Specification { return className.replace('.', '/') + ".class" } + @CompileStatic private static class ParentClassLoader extends URLClassLoader { ParentClassLoader() {