From 8e0c307e54d8fdf87c5b9447c4c53a71e6502715 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Xavier=20L=C3=A9aut=C3=A9?= Date: Sat, 24 Aug 2019 20:47:54 -0400 Subject: [PATCH] Do not assume system classloader is URLClassLoader in Java 9+ (#8392) * Fallback to parsing classpath for hadoop task in Java 9+ In Java 9 and above we cannot assume that the system classloader is an instance of URLClassLoader. This change adds a fallback method to parse the system classpath in that case, and adds a unit test to validate it matches what JDK8 would do. Note: This has not been tested in an actual hadoop setup, so this is mostly to help us pass unit tests. * Remove granularity test of dubious value One of our granularity tests relies on system classloader being a URLClassLoaders to catch a bug related to class initialization and static initializers using a subclass (see #2979) This test was added to catch a potential regression, but it assumes we would add back the same type of static initializers to this specific class, so it seems to be of dubious value as a unit test and mostly serves to illustrate the bug. relates to #5589 --- core/pom.xml | 2 ++ .../java/org/apache/druid/utils/JvmUtils.java | 24 +++++++++++++++++++ .../org/apache/druid/utils/JvmUtilsTest.java | 19 +++++++++++++++ .../indexing/common/task/HadoopTask.java | 23 ++++++++++++------ .../indexing/common/task/HadoopTaskTest.java | 1 + .../granularity/QueryGranularityTest.java | 13 ---------- 6 files changed, 62 insertions(+), 20 deletions(-) diff --git a/core/pom.xml b/core/pom.xml index edb9bdc8caa..a06b082189e 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -344,6 +344,8 @@ org.apache.maven.plugins maven-surefire-plugin + + false @{jacocoArgLine} -Djava.library.path=${project.build.directory}/hyperic-sigar-${sigar.base.version}/sigar-bin/lib/ diff --git a/core/src/main/java/org/apache/druid/utils/JvmUtils.java b/core/src/main/java/org/apache/druid/utils/JvmUtils.java index d3e830a09ac..7211545c4e2 100644 --- a/core/src/main/java/org/apache/druid/utils/JvmUtils.java +++ b/core/src/main/java/org/apache/druid/utils/JvmUtils.java @@ -21,9 +21,16 @@ package org.apache.druid.utils; import com.google.inject.Inject; +import java.io.File; import java.lang.management.ManagementFactory; import java.lang.management.ThreadMXBean; +import java.net.MalformedURLException; +import java.net.URL; +import java.nio.file.Paths; +import java.util.List; import java.util.StringTokenizer; +import java.util.stream.Collectors; +import java.util.stream.Stream; public class JvmUtils { @@ -79,4 +86,21 @@ public class JvmUtils { return THREAD_MX_BEAN.getCurrentThreadCpuTime(); } + + public static List systemClassPath() + { + List jobURLs; + String[] paths = System.getProperty("java.class.path").split(File.pathSeparator); + jobURLs = Stream.of(paths).map( + s -> { + try { + return Paths.get(s).toUri().toURL(); + } + catch (MalformedURLException e) { + throw new UnsupportedOperationException("Unable to create URL classpath entry", e); + } + } + ).collect(Collectors.toList()); + return jobURLs; + } } diff --git a/core/src/test/java/org/apache/druid/utils/JvmUtilsTest.java b/core/src/test/java/org/apache/druid/utils/JvmUtilsTest.java index c9e7599e0b1..6136b7a0943 100644 --- a/core/src/test/java/org/apache/druid/utils/JvmUtilsTest.java +++ b/core/src/test/java/org/apache/druid/utils/JvmUtilsTest.java @@ -20,8 +20,14 @@ package org.apache.druid.utils; import org.junit.Assert; +import org.junit.Assume; import org.junit.Test; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.Arrays; +import java.util.List; + public class JvmUtilsTest { @Test @@ -38,4 +44,17 @@ public class JvmUtilsTest Assert.assertTrue(true); } } + + @Test + public void testSystemClassPath() + { + ClassLoader testClassLoader = this.getClass().getClassLoader(); + // ignore this test unless we can assume URLClassLoader (only applies to Java 8) + Assume.assumeTrue(testClassLoader instanceof URLClassLoader); + + List parsedUrls = JvmUtils.systemClassPath(); + List classLoaderUrls = Arrays.asList(((URLClassLoader) testClassLoader).getURLs()); + + Assert.assertEquals(classLoaderUrls, parsedUrls); + } } diff --git a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java index 8bbe5ca65d4..eba75fce099 100644 --- a/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java +++ b/indexing-service/src/main/java/org/apache/druid/indexing/common/task/HadoopTask.java @@ -29,6 +29,7 @@ import org.apache.druid.guice.GuiceInjectors; import org.apache.druid.indexing.common.TaskToolbox; import org.apache.druid.initialization.Initialization; import org.apache.druid.java.util.common.logger.Logger; +import org.apache.druid.utils.JvmUtils; import javax.annotation.Nullable; import java.io.File; @@ -138,14 +139,22 @@ public abstract class HadoopTask extends AbstractBatchIndexTask ? hadoopDependencyCoordinates : defaultHadoopCoordinates; - final List jobURLs = Lists.newArrayList( - Arrays.asList(((URLClassLoader) HadoopIndexTask.class.getClassLoader()).getURLs()) - ); + ClassLoader taskClassLoader = HadoopIndexTask.class.getClassLoader(); + final List jobURLs; + if (taskClassLoader instanceof URLClassLoader) { + // this only works with Java 8 + jobURLs = Lists.newArrayList( + Arrays.asList(((URLClassLoader) taskClassLoader).getURLs()) + ); + } else { + // fallback to parsing system classpath + jobURLs = JvmUtils.systemClassPath(); + } final List extensionURLs = new ArrayList<>(); for (final File extension : Initialization.getExtensionFilesToLoad(EXTENSIONS_CONFIG)) { - final ClassLoader extensionLoader = Initialization.getClassLoaderForExtension(extension, false); - extensionURLs.addAll(Arrays.asList(((URLClassLoader) extensionLoader).getURLs())); + final URLClassLoader extensionLoader = Initialization.getClassLoaderForExtension(extension, false); + extensionURLs.addAll(Arrays.asList(extensionLoader.getURLs())); } jobURLs.addAll(extensionURLs); @@ -158,8 +167,8 @@ public abstract class HadoopTask extends AbstractBatchIndexTask finalHadoopDependencyCoordinates, EXTENSIONS_CONFIG )) { - final ClassLoader hadoopLoader = Initialization.getClassLoaderForExtension(hadoopDependency, false); - localClassLoaderURLs.addAll(Arrays.asList(((URLClassLoader) hadoopLoader).getURLs())); + final URLClassLoader hadoopLoader = Initialization.getClassLoaderForExtension(hadoopDependency, false); + localClassLoaderURLs.addAll(Arrays.asList(hadoopLoader.getURLs())); } final ClassLoader classLoader = new URLClassLoader( diff --git a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java index 2ec6b1200e8..409e7d0f669 100644 --- a/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java +++ b/indexing-service/src/test/java/org/apache/druid/indexing/common/task/HadoopTaskTest.java @@ -128,6 +128,7 @@ public class HadoopTaskTest final Class druidHadoopConfigClazz = Class.forName("org.apache.druid.indexer.HadoopDruidIndexerConfig", false, classLoader); assertClassLoaderIsSingular(druidHadoopConfigClazz.getClassLoader()); } + public static void assertClassLoaderIsSingular(ClassLoader classLoader) { // This is a check against the current HadoopTask which creates a single URLClassLoader with null parent diff --git a/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java b/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java index 46770544bb9..8f7853f413e 100644 --- a/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java +++ b/processing/src/test/java/org/apache/druid/granularity/QueryGranularityTest.java @@ -46,8 +46,6 @@ import org.joda.time.Years; import org.junit.Assert; import org.junit.Test; -import java.net.URL; -import java.net.URLClassLoader; import java.util.Collections; import java.util.Iterator; import java.util.List; @@ -802,17 +800,6 @@ public class QueryGranularityTest Assert.assertFalse("actualIter not exhausted!?", actualIter.hasNext()); Assert.assertFalse("expectedIter not exhausted!?", expectedIter.hasNext()); } - - @Test(timeout = 60_000L) - public void testDeadLock() throws Exception - { - final URL[] urls = ((URLClassLoader) Granularity.class.getClassLoader()).getURLs(); - final String className = Granularity.class.getName(); - for (int i = 0; i < 1000; ++i) { - final ClassLoader loader = new URLClassLoader(urls, null); - Assert.assertNotNull(String.valueOf(i), Class.forName(className, true, loader)); - } - } @Test public void testTruncateKathmandu()