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
This commit is contained in:
Xavier Léauté 2019-08-24 20:47:54 -04:00 committed by GitHub
parent 20f7db5d22
commit 8e0c307e54
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 62 additions and 20 deletions

View File

@ -344,6 +344,8 @@
<groupId>org.apache.maven.plugins</groupId> <groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId> <artifactId>maven-surefire-plugin</artifactId>
<configuration> <configuration>
<!-- use normal classpath instead of manifest jar for JvmUtilsTest.testSystemClassPath -->
<useManifestOnlyJar>false</useManifestOnlyJar>
<argLine> <argLine>
@{jacocoArgLine} @{jacocoArgLine}
-Djava.library.path=${project.build.directory}/hyperic-sigar-${sigar.base.version}/sigar-bin/lib/ -Djava.library.path=${project.build.directory}/hyperic-sigar-${sigar.base.version}/sigar-bin/lib/

View File

@ -21,9 +21,16 @@ package org.apache.druid.utils;
import com.google.inject.Inject; import com.google.inject.Inject;
import java.io.File;
import java.lang.management.ManagementFactory; import java.lang.management.ManagementFactory;
import java.lang.management.ThreadMXBean; 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.StringTokenizer;
import java.util.stream.Collectors;
import java.util.stream.Stream;
public class JvmUtils public class JvmUtils
{ {
@ -79,4 +86,21 @@ public class JvmUtils
{ {
return THREAD_MX_BEAN.getCurrentThreadCpuTime(); return THREAD_MX_BEAN.getCurrentThreadCpuTime();
} }
public static List<URL> systemClassPath()
{
List<URL> 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;
}
} }

View File

@ -20,8 +20,14 @@
package org.apache.druid.utils; package org.apache.druid.utils;
import org.junit.Assert; import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test; import org.junit.Test;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Arrays;
import java.util.List;
public class JvmUtilsTest public class JvmUtilsTest
{ {
@Test @Test
@ -38,4 +44,17 @@ public class JvmUtilsTest
Assert.assertTrue(true); 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<URL> parsedUrls = JvmUtils.systemClassPath();
List<URL> classLoaderUrls = Arrays.asList(((URLClassLoader) testClassLoader).getURLs());
Assert.assertEquals(classLoaderUrls, parsedUrls);
}
} }

View File

@ -29,6 +29,7 @@ import org.apache.druid.guice.GuiceInjectors;
import org.apache.druid.indexing.common.TaskToolbox; import org.apache.druid.indexing.common.TaskToolbox;
import org.apache.druid.initialization.Initialization; import org.apache.druid.initialization.Initialization;
import org.apache.druid.java.util.common.logger.Logger; import org.apache.druid.java.util.common.logger.Logger;
import org.apache.druid.utils.JvmUtils;
import javax.annotation.Nullable; import javax.annotation.Nullable;
import java.io.File; import java.io.File;
@ -138,14 +139,22 @@ public abstract class HadoopTask extends AbstractBatchIndexTask
? hadoopDependencyCoordinates ? hadoopDependencyCoordinates
: defaultHadoopCoordinates; : defaultHadoopCoordinates;
final List<URL> jobURLs = Lists.newArrayList( ClassLoader taskClassLoader = HadoopIndexTask.class.getClassLoader();
Arrays.asList(((URLClassLoader) HadoopIndexTask.class.getClassLoader()).getURLs()) final List<URL> 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<URL> extensionURLs = new ArrayList<>(); final List<URL> extensionURLs = new ArrayList<>();
for (final File extension : Initialization.getExtensionFilesToLoad(EXTENSIONS_CONFIG)) { for (final File extension : Initialization.getExtensionFilesToLoad(EXTENSIONS_CONFIG)) {
final ClassLoader extensionLoader = Initialization.getClassLoaderForExtension(extension, false); final URLClassLoader extensionLoader = Initialization.getClassLoaderForExtension(extension, false);
extensionURLs.addAll(Arrays.asList(((URLClassLoader) extensionLoader).getURLs())); extensionURLs.addAll(Arrays.asList(extensionLoader.getURLs()));
} }
jobURLs.addAll(extensionURLs); jobURLs.addAll(extensionURLs);
@ -158,8 +167,8 @@ public abstract class HadoopTask extends AbstractBatchIndexTask
finalHadoopDependencyCoordinates, finalHadoopDependencyCoordinates,
EXTENSIONS_CONFIG EXTENSIONS_CONFIG
)) { )) {
final ClassLoader hadoopLoader = Initialization.getClassLoaderForExtension(hadoopDependency, false); final URLClassLoader hadoopLoader = Initialization.getClassLoaderForExtension(hadoopDependency, false);
localClassLoaderURLs.addAll(Arrays.asList(((URLClassLoader) hadoopLoader).getURLs())); localClassLoaderURLs.addAll(Arrays.asList(hadoopLoader.getURLs()));
} }
final ClassLoader classLoader = new URLClassLoader( final ClassLoader classLoader = new URLClassLoader(

View File

@ -128,6 +128,7 @@ public class HadoopTaskTest
final Class<?> druidHadoopConfigClazz = Class.forName("org.apache.druid.indexer.HadoopDruidIndexerConfig", false, classLoader); final Class<?> druidHadoopConfigClazz = Class.forName("org.apache.druid.indexer.HadoopDruidIndexerConfig", false, classLoader);
assertClassLoaderIsSingular(druidHadoopConfigClazz.getClassLoader()); assertClassLoaderIsSingular(druidHadoopConfigClazz.getClassLoader());
} }
public static void assertClassLoaderIsSingular(ClassLoader classLoader) public static void assertClassLoaderIsSingular(ClassLoader classLoader)
{ {
// This is a check against the current HadoopTask which creates a single URLClassLoader with null parent // This is a check against the current HadoopTask which creates a single URLClassLoader with null parent

View File

@ -46,8 +46,6 @@ import org.joda.time.Years;
import org.junit.Assert; import org.junit.Assert;
import org.junit.Test; import org.junit.Test;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Collections; import java.util.Collections;
import java.util.Iterator; import java.util.Iterator;
import java.util.List; import java.util.List;
@ -803,17 +801,6 @@ public class QueryGranularityTest
Assert.assertFalse("expectedIter not exhausted!?", expectedIter.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 @Test
public void testTruncateKathmandu() public void testTruncateKathmandu()
{ {