Use a dedicated classLoader for plugins, instead of (ab)using the sys…

…tem class loader

The plugin class loader creates a special ClassLoader. This class loader is used (by means of static assignment) on all places where previously a `Class.forName` was used.

src/fitnesse/components/Logger.java

@@ -3,6 +3,8 @@
 
 package fitnesse.components;
 
+import org.apache.commons.lang.ClassUtils;
+
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileOutputStream;

src/fitnesse/components/PluginsClassLoader.java

@@ -2,45 +2,53 @@
 
 import java.io.File;
 import java.lang.reflect.Method;
+import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.util.ArrayList;
+import java.util.List;
 
 
+/**
+ * Update the current thread class path with jars foundin a "plugins" directory.
+ */
 public class PluginsClassLoader {
 
-  private File pluginsDirectory;
+  public static ClassLoader loadPlugins(String rootPath) throws Exception {
+    File pluginsDirectory = new File(rootPath, "plugins");
 
-  public PluginsClassLoader(String rootPath) {
-    pluginsDirectory = new File(rootPath, "plugins");
+    URL[] urls = urlsForPlugins(pluginsDirectory);
+    ClassLoader newCL = new URLClassLoader(urls, Thread.currentThread().getContextClassLoader());
+
+//    Thread.currentThread().setContextClassLoader(newCL);
+    return newCL;
   }
 
-  public void addPluginsToClassLoader() throws Exception {
-    if (pluginsDirectory.exists())
+  private static URL[] urlsForPlugins(File pluginsDirectory) throws Exception {
+    List<URL> urls = new ArrayList<>();
+
+    if (pluginsDirectory.exists() && pluginsDirectory.isDirectory())
       for (File plugin : pluginsDirectory.listFiles())
         if (plugin.getName().endsWith("jar"))
-          addItemsToClasspath(plugin.getCanonicalPath());
+          urls.addAll(toUrls(plugin.getCanonicalPath()));
+
+    return urls.toArray(new URL[urls.size()]);
   }
 
-  public static void addItemsToClasspath(String classpathItems) throws Exception {
+  private static List<URL> toUrls(String classpathItems) throws Exception {
     final String separator = File.pathSeparator;
     String currentClassPath = System.getProperty("java.class.path");
     System.setProperty("java.class.path", currentClassPath + separator + classpathItems);
     String[] items = classpathItems.split(separator);
+    List<URL> urls = new ArrayList<>(items.length);
+
     for (String item : items) {
-      addFileToClassPath(item);
+      urls.add(toUrl(item));
     }
+    return urls;
   }
 
-  private static void addFileToClassPath(String fileName) throws Exception {
-    addUrlToClasspath(new File(fileName).toURI().toURL());
+  private static URL toUrl(String fileName) throws MalformedURLException {
+    return new File(fileName).toURI().toURL();
   }
-
-  public static void addUrlToClasspath(URL u) throws Exception {
-    URLClassLoader sysLoader = (URLClassLoader) ClassLoader.getSystemClassLoader();
-    Class<URLClassLoader> sysClass = URLClassLoader.class;
-    Method method = sysClass.getDeclaredMethod("addURL", new Class[]{URL.class});
-    method.setAccessible(true);
-    method.invoke(sysLoader, new Object[]{u});
-  }
-
 }

src/fitnesse/plugins/PluginsLoader.java

@@ -19,6 +19,7 @@
 import fitnesse.testrunner.TestSystemFactoryRegistry;
 import fitnesse.testsystems.slim.CustomComparatorRegistry;
 import fitnesse.testsystems.slim.tables.SlimTableFactory;
+import fitnesse.util.ClassUtils;
 import fitnesse.wiki.WikiPageFactoryRegistry;
 import fitnesse.wikitext.parser.SymbolProvider;
 
@@ -37,7 +38,7 @@ private Collection<PluginFeatureFactory> findPluginFeatureFactories() throws Plu
     List<PluginFeatureFactory> factories = new ArrayList<>();
     factories.addAll(PropertyBasedPluginFeatureFactory.loadFromProperties(componentFactory));
 
-    for (PluginFeatureFactory factory : ServiceLoader.load(PluginFeatureFactory.class)) {
+    for (PluginFeatureFactory factory : ServiceLoader.load(PluginFeatureFactory.class, ClassUtils.getClassLoader())) {
       factories.add(factory);
     }
     return factories;

src/fitnesse/util/ClassUtils.java

@@ -2,12 +2,21 @@
 
 public class ClassUtils {
 
+  private static ClassLoader classLoader;
+
   private ClassUtils() {
   }
 
   @SuppressWarnings("unchecked")
   public static <T> Class<T> forName(String className) throws ClassNotFoundException {
-    return (Class<T>) Class.forName(className);
+    return (Class<T>) (classLoader == null ? Class.forName(className) : Class.forName(className, true, classLoader));
+  }
+
+  public static void setClassLoader(ClassLoader classLoader) {
+    ClassUtils.classLoader = classLoader;
   }
 
+  public static ClassLoader getClassLoader() {
+    return classLoader;
+  }
 }

src/fitnesseMain/FitNesseMain.java

@@ -10,6 +10,7 @@
 import fitnesse.socketservice.PlainServerSocketFactory;
 import fitnesse.socketservice.SslServerSocketFactory;
 import fitnesse.updates.WikiContentUpdater;
+import fitnesse.util.ClassUtils;
 
 import java.io.*;
 import java.net.BindException;
@@ -119,7 +120,8 @@ private boolean update(FitNesseContext context) throws IOException {
   }
 
   private void loadPlugins(String rootPath) throws Exception {
-    new PluginsClassLoader(rootPath).addPluginsToClassLoader();
+    ClassLoader classLoader = new PluginsClassLoader().loadPlugins(rootPath);
+    ClassUtils.setClassLoader(classLoader);
   }
 
   private Integer launch(FitNesseContext context) throws Exception {

test/fitnesse/components/PluginsClassLoaderTest.java

@@ -16,66 +16,29 @@ public class PluginsClassLoaderTest {
 
   @Test
   public void whenPluginsDirectoryDoesNotExist() throws Exception {
-    PluginsClassLoader pluginsClassLoader = new PluginsClassLoader("nonExistingRootDirectory");
-    pluginsClassLoader.addPluginsToClassLoader();
+    new PluginsClassLoader().loadPlugins("nonExistingRootDirectory");
+
     assertTrue("didn't cause exception", true);
   }
 
   @Test
   public void addPluginsToClassLoader() throws Exception {
     String[] dynamicClasses = new String[]{"fitnesse.testing.PluginX", "fitnesse.testing.PluginY"};
     //todo This fails because some other test probably loads plugin path    assertLoadingClassCausesException(dynamicClasses);
-    PluginsClassLoader pluginsClassLoader = new PluginsClassLoader(".");
-    pluginsClassLoader.addPluginsToClassLoader();
-    assertLoadingClassWorksNow(dynamicClasses);
+    ClassLoader cl = PluginsClassLoader.loadPlugins(".");
+
+
+    assertLoadingClassWorksNow(cl, dynamicClasses);
   }
 
-  private void assertLoadingClassWorksNow(String... dynamicClasses) {
+  private void assertLoadingClassWorksNow(ClassLoader cl, String... dynamicClasses) {
     for (String dynamicClass : dynamicClasses) {
       try {
-        Class<?> dynamicallyLoadedClass = Class.forName(dynamicClass);
+        Class<?> dynamicallyLoadedClass = Class.forName(dynamicClass, true, cl);
         assertEquals(dynamicClass, dynamicallyLoadedClass.getName());
       } catch (ClassNotFoundException e) {
-        fail(e.getMessage());
+        fail("Class not found: " + e.getMessage());
       }
     }
   }
-
-  @Test
-  public void testAddUrlToClasspath() throws Exception {
-    ClassLoader systemClassLoader = ClassLoader.getSystemClassLoader();
-    assertTrue(systemClassLoader instanceof URLClassLoader);
-    URLClassLoader classLoader = (URLClassLoader) systemClassLoader;
-
-    URL sampleUrl = new File("src").toURI().toURL();
-
-    String classpath = classpathAsString(classLoader);
-    assertNotSubString(sampleUrl.toString(), classpath);
-
-    PluginsClassLoader.addUrlToClasspath(sampleUrl);
-    classpath = classpathAsString(classLoader);
-    assertSubString(sampleUrl.toString(), classpath);
-  }
-
-  @Test
-  public void testAddMultipleUrlsToClasspath() throws Exception {
-    String separator = System.getProperty("path.separator");
-    String paths = "/blah/blah" + separator + "C" + otherSeperator(separator) + "\\foo\\bar";
-    PluginsClassLoader.addItemsToClasspath(paths);
-    URLClassLoader classLoader = (URLClassLoader) ClassLoader.getSystemClassLoader();
-    String classpath = classpathAsString(classLoader);
-    assertSubString("/blah/blah", classpath);
-    assertMatches("[C" + otherSeperator(separator) + "?foo?bar]", classpath);
-  }
-
-  private String otherSeperator(String separator) {
-    return separator.equals(";") ? ":" : ";";
-  }
-
-  private String classpathAsString(URLClassLoader classLoader) {
-    URL[] urls = classLoader.getURLs();
-    return StringUtils.join(urls, ":");
-  }
-
-
 }

test/fitnesse/junit/FitNesseRunnerExtensionTest.java

@@ -41,7 +41,7 @@ protected String getSuiteName(Class<?> klass) throws InitializationError {
 
     @Override
     protected FitNesseContext createContext(Class<?> suiteClass) throws Exception {
-      new PluginsClassLoader(getFitNesseRoot(suiteClass)).addPluginsToClassLoader();
+      new PluginsClassLoader().loadPlugins(getFitNesseRoot(suiteClass));
 
       return super.createContext(suiteClass);
     }

test/fitnesse/plugins/PluginsLoaderTest.java

@@ -4,6 +4,7 @@
 
 import java.io.File;
 import java.net.URL;
+import java.net.URLClassLoader;
 import java.util.List;
 import java.util.Properties;
 
@@ -36,6 +37,7 @@
 import fitnesse.testsystems.slim.tables.SlimTable;
 import fitnesse.testsystems.slim.tables.SlimTableFactory;
 import fitnesse.testutil.SimpleAuthenticator;
+import fitnesse.util.ClassUtils;
 import fitnesse.wiki.WikiPage;
 import fitnesse.wiki.WikiPageDummy;
 import fitnesse.wiki.WikiPageFactory;
@@ -55,6 +57,7 @@
 import org.htmlparser.tags.TableRow;
 import org.htmlparser.tags.TableTag;
 import org.htmlparser.util.NodeList;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -81,14 +84,21 @@ public void setUp() throws Exception {
     testCustomComparatorsRegistry = new CustomComparatorRegistry();
     testTestSystemFactory = new MultipleTestSystemFactory(testSlimTableFactory, testCustomComparatorsRegistry);
 
-    URL sampleUrl = new File("test/fitnesse/plugins").toURI().toURL();
-    PluginsClassLoader.addUrlToClasspath(sampleUrl);
+    URL pluginLoaderTestDirectory = new File("plugin-loader-test").toURI().toURL();
+
+    ClassLoader cl = new URLClassLoader(new URL[] { pluginLoaderTestDirectory }, ClassLoader.getSystemClassLoader());
+    ClassUtils.setClassLoader(cl);
 
     loader = new PluginsLoader(new ComponentFactory(testProperties));
 
     assertSymbolTypeMatch("!today", false);
   }
 
+  @After
+  public void tearDown() {
+    ClassUtils.setClassLoader(null);
+  }
+
   @Test
   public void testAddPlugins() throws Exception {
     testProperties.setProperty(ConfigurationParameter.PLUGINS.getKey(), DummyPlugin.class.getName());