[incubator-druid] branch master updated: Do not assume system classloader is URLClassLoader in Java 9+ (#8392)

[xv...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

xvrl pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-druid.git


The following commit(s) were added to refs/heads/master by this push:
     new 8e0c307  Do not assume system classloader is URLClassLoader in Java 9+ (#8392)
8e0c307 is described below

commit 8e0c307e54d8fdf87c5b9447c4c53a71e6502715
Author: Xavier Léauté <xv...@apache.org>
AuthorDate: Sat Aug 24 20:47:54 2019 -0400

    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 ++
 .../main/java/org/apache/druid/utils/JvmUtils.java | 24 ++++++++++++++++++++++
 .../java/org/apache/druid/utils/JvmUtilsTest.java  | 19 +++++++++++++++++
 .../druid/indexing/common/task/HadoopTask.java     | 23 ++++++++++++++-------
 .../druid/indexing/common/task/HadoopTaskTest.java |  1 +
 .../druid/granularity/QueryGranularityTest.java    | 13 ------------
 6 files changed, 62 insertions(+), 20 deletions(-)

diff --git a/core/pom.xml b/core/pom.xml
index edb9bdc..a06b082 100644
--- a/core/pom.xml
+++ b/core/pom.xml
@@ -344,6 +344,8 @@
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-surefire-plugin</artifactId>
         <configuration>
+          <!-- use normal classpath instead of manifest jar for JvmUtilsTest.testSystemClassPath -->
+          <useManifestOnlyJar>false</useManifestOnlyJar>
           <argLine>
             @{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 d3e830a..7211545 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<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;
+  }
 }
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 c9e7599..6136b7a 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<URL> parsedUrls = JvmUtils.systemClassPath();
+    List<URL> 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 8bbe5ca..eba75fc 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<URL> jobURLs = Lists.newArrayList(
-        Arrays.asList(((URLClassLoader) HadoopIndexTask.class.getClassLoader()).getURLs())
-    );
+    ClassLoader taskClassLoader = HadoopIndexTask.class.getClassLoader();
+    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<>();
     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 2ec6b12..409e7d0 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 4677054..8f7853f 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()


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org