MODULES-241 Leaked Alias Modules

From now on, alias modules are handled as regular modules with a single dependency on the alias target
fl4via · Dec 7, 2016 · ff8b78c · ff8b78c
1 parent 19fe543
commit ff8b78c
Show file tree

Hide file tree

Showing 22 changed files with 504 additions and 12 deletions.
diff --git a/src/main/java/org/jboss/modules/AliasModuleSpec.java b/src/main/java/org/jboss/modules/AliasModuleSpec.java
@@ -18,18 +18,48 @@
 
 package org.jboss.modules;
 
+import java.lang.instrument.ClassFileTransformer;
+import java.security.PermissionCollection;
+import java.util.Collections;
+import java.util.Map;
+
+import org.jboss.modules.filter.PathFilters;
+
 /**
  * A module specification for a module alias.
+ * <p>
+ * Note that because of MODULES-241, alias modules are handled as a regular modules with a single dependency on the alias
+ * target. Use {@link #asConcreteModuleSpec()} to get the regular {@link ConcreteModuleSpec} representing this
+ * {@link AliasModuleSpec}.
  *
  * @author <a href="mailto:david.lloyd@redhat.com">David M. Lloyd</a>
+ * @author <a href="https://github.com/ppalaga">Peter Palaga</a>
  */
 public final class AliasModuleSpec extends ModuleSpec {
 
     private final String aliasName;
+    private final ConcreteModuleSpec concreteModuleSpec;
 
     AliasModuleSpec(final String name, final String aliasName) {
         super(name);
         this.aliasName = aliasName;
+
+        DependencySpec aliasTargetDependency = DependencySpec.createModuleDependencySpec(PathFilters.acceptAll(),
+                PathFilters.acceptAll(), null, aliasName, false);
+
+        final String mainClass = null;
+        final AssertionSetting assertionSetting = AssertionSetting.INHERIT;
+        final ResourceLoaderSpec[] resourceLoaders = ResourceLoaderSpec.NO_RESOURCE_LOADERS;
+        final DependencySpec[] dependencies = new DependencySpec[] { aliasTargetDependency };
+        final LocalLoader fallbackLoader = null;
+        final ModuleClassLoaderFactory moduleClassLoaderFactory = null;
+        final ClassFileTransformer classFileTransformer = null;
+        final Map<String, String> properties = Collections.emptyMap();
+        final PermissionCollection permissionCollection = ModulesPolicy.DEFAULT_PERMISSION_COLLECTION;
+        final Version version = null;
+        this.concreteModuleSpec = new ConcreteModuleSpec(name, mainClass, assertionSetting, resourceLoaders,
+                dependencies, fallbackLoader, moduleClassLoaderFactory, classFileTransformer, properties, permissionCollection,
+                version);
     }
 
     /**
@@ -51,4 +81,13 @@ public ModuleIdentifier getAliasTarget() {
     public String getAliasName() {
         return aliasName;
     }
+
+    /**
+     * @return a {@link ConcreteModuleSpec} instance representing this {@link AliasModuleSpec} - i.e. a
+     *         {@link ConcreteModuleSpec} with single dependency on the alias target, with the import and export filters set to
+     *         {@link PathFilters#acceptAll()}.
+     */
+    ConcreteModuleSpec asConcreteModuleSpec() {
+        return concreteModuleSpec;
+    }
 }
diff --git a/src/main/java/org/jboss/modules/ModuleClassLoader.java b/src/main/java/org/jboss/modules/ModuleClassLoader.java
@@ -66,8 +66,6 @@ public class ModuleClassLoader extends ConcurrentClassLoader {
         }
     }
 
-    static final ResourceLoaderSpec[] NO_RESOURCE_LOADERS = new ResourceLoaderSpec[0];
-
     private final Module module;
     private final ClassFileTransformer transformer;
 
@@ -130,7 +128,7 @@ protected ModuleClassLoader(final Configuration configuration) {
      */
     boolean recalculate() {
         final Paths<ResourceLoader, ResourceLoaderSpec> paths = this.paths.get();
-        return setResourceLoaders(paths, paths.getSourceList(NO_RESOURCE_LOADERS));
+        return setResourceLoaders(paths, paths.getSourceList(ResourceLoaderSpec.NO_RESOURCE_LOADERS));
     }
 
     /**
@@ -520,7 +518,7 @@ protected final String findLibrary(final String libname) {
         final ModuleLogger log = Module.log;
         log.trace("Attempting to load native library %s from %s", libname, module);
 
-        for (ResourceLoaderSpec loader : paths.get().getSourceList(NO_RESOURCE_LOADERS)) {
+        for (ResourceLoaderSpec loader : paths.get().getSourceList(ResourceLoaderSpec.NO_RESOURCE_LOADERS)) {
             final String library = loader.getResourceLoader().getLibrary(libname);
             if (library != null) {
                 return library;
@@ -654,7 +652,7 @@ protected final void finalize() throws Throwable {
     }
 
     ResourceLoader[] getResourceLoaders() {
-        final ResourceLoaderSpec[] specs = paths.get().getSourceList(NO_RESOURCE_LOADERS);
+        final ResourceLoaderSpec[] specs = paths.get().getSourceList(ResourceLoaderSpec.NO_RESOURCE_LOADERS);
         final int length = specs.length;
         final ResourceLoader[] loaders = new ResourceLoader[length];
         for (int i = 0; i < length; i++) {

diff --git a/src/main/java/org/jboss/modules/ModuleLoader.java b/src/main/java/org/jboss/modules/ModuleLoader.java
@@ -494,13 +494,7 @@ protected final Module loadModuleLocal(String name) throws ModuleLoadException {
             }
             final Module module;
             if ( moduleSpec instanceof AliasModuleSpec) {
-                final String aliasName = ((AliasModuleSpec) moduleSpec).getAliasName();
-                try {
-                    newFuture.setModule(module = loadModuleLocal(aliasName));
-                } catch (RuntimeException | Error e) {
-                    log.trace(e, "Failed to load module %s (alias for %s)", name, aliasName);
-                    throw e;
-                }
+                module = defineModule(((AliasModuleSpec) moduleSpec).asConcreteModuleSpec(), newFuture);
             } else {
                 module = defineModule((ConcreteModuleSpec) moduleSpec, newFuture);
             }

diff --git a/src/main/java/org/jboss/modules/ResourceLoaderSpec.java b/src/main/java/org/jboss/modules/ResourceLoaderSpec.java
@@ -30,6 +30,7 @@
  * @author <a href="mailto:david.lloyd@redhat.com">David M. Lloyd</a>
  */
 public final class ResourceLoaderSpec {
+    static final ResourceLoaderSpec[] NO_RESOURCE_LOADERS = new ResourceLoaderSpec[0];
     private final ResourceLoader resourceLoader;
     private final PathFilter pathFilter;
 

diff --git a/src/test/java/org/jboss/modules/ModuleClassLoaderAliasReloadTest.java b/src/test/java/org/jboss/modules/ModuleClassLoaderAliasReloadTest.java
@@ -0,0 +1,164 @@
+package org.jboss.modules;
+
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.util.Collection;
+
+import org.jboss.modules.test.TestClass;
+import org.jboss.modules.util.TestModuleLoader;
+import org.jboss.modules.util.TestResourceLoader;
+import org.junit.Test;
+
+/**
+ * Verifies the functionality of alias modules in unload/reload scenarios.
+ *
+ * @author <a href="mailto:ropalka@redhat.com">Richard Opalka</a>
+ * @author <a href="https://github.com/ppalaga">Peter Palaga</a>
+ */
+public class ModuleClassLoaderAliasReloadTest extends AbstractModuleTestCase {
+
+    private static final ModuleIdentifier MODULE_ONE_ID = ModuleIdentifier.fromString("test-module-1");
+    private static final ModuleIdentifier MODULE_TWO_ID = ModuleIdentifier.fromString("test-module-2");
+    private static final ModuleIdentifier MODULE_TWO_AL = ModuleIdentifier.fromString("test-module-2-alias");
+    private TestModuleLoader moduleLoader = new TestModuleLoader();
+
+    private static final class CloseAwareResourceLoader implements ResourceLoader {
+
+        private final ResourceLoader delegate;
+        private volatile boolean closed;
+
+        private CloseAwareResourceLoader(final ResourceLoader delegate) {
+            this.delegate = delegate;
+        }
+
+        @Override
+        public String getRootName() {
+            if (closed) throw new IllegalStateException();
+            return delegate.getRootName();
+        }
+
+        @Override
+        public ClassSpec getClassSpec(String fileName) throws IOException {
+            if (closed) throw new IllegalStateException();
+            return delegate.getClassSpec(fileName);
+        }
+
+        @Override
+        public PackageSpec getPackageSpec(String name) throws IOException {
+            if (closed) throw new IllegalStateException();
+            return delegate.getPackageSpec(name);
+        }
+
+        @Override
+        public Resource getResource(String name) {
+            if (closed) throw new IllegalStateException();
+            return delegate.getResource(name);
+        }
+
+        @Override
+        public String getLibrary(String name) {
+            if (closed) throw new IllegalStateException();
+            return delegate.getLibrary(name);
+        }
+
+        @Override
+        public Collection<String> getPaths() {
+            if (closed) throw new IllegalStateException();
+            return delegate.getPaths();
+        }
+
+        public void close() {
+            closed = true;
+        }
+    }
+
+    public void configureModules() throws Exception {
+        // first module
+        final ModuleSpec.Builder moduleOneBuilder = ModuleSpec.build(MODULE_ONE_ID);
+        moduleOneBuilder.addResourceRoot(ResourceLoaderSpec.createResourceLoaderSpec(
+                new CloseAwareResourceLoader(
+                        TestResourceLoader.build()
+                                .addClass(TestClass.class)
+                                .addResources(getResource("test/aliasmodule/rootOne"))
+                                .create())
+        ));
+        moduleOneBuilder.addDependency(DependencySpec.createModuleDependencySpec(MODULE_TWO_AL));
+        moduleOneBuilder.addDependency(DependencySpec.createLocalDependencySpec());
+        moduleLoader.addModuleSpec(moduleOneBuilder.create());
+        // second module
+        final ModuleSpec.Builder moduleTwoBuilder = ModuleSpec.build(MODULE_TWO_ID);
+        moduleTwoBuilder.addResourceRoot(ResourceLoaderSpec.createResourceLoaderSpec(
+                new CloseAwareResourceLoader(
+                        TestResourceLoader.build()
+                                .addClass(TestClass.class)
+                                .addResources(getResource("test/aliasmodule/rootTwo"))
+                                .create()
+                )
+        ));
+        moduleTwoBuilder.addDependency(DependencySpec.createModuleDependencySpec(MODULE_ONE_ID));
+        moduleTwoBuilder.addDependency(DependencySpec.createLocalDependencySpec());
+        moduleLoader.addModuleSpec(moduleTwoBuilder.create());
+        // second alias module
+        final ModuleSpec.AliasBuilder moduleTwoAliasBuilder = ModuleSpec.buildAlias(MODULE_TWO_AL, MODULE_TWO_ID);
+        moduleLoader.addModuleSpec(moduleTwoAliasBuilder.create());
+    }
+
+    /**
+     * Ensure the Alias modules do not leak during unload/reload, see https://issues.jboss.org/browse/MODULES-241
+     *
+     * @throws Exception
+     */
+    @Test
+    public void testAliasModuleReload() throws Exception {
+        // FIRST PHASE
+        configureModules();
+        Module testModule1 = moduleLoader.loadModule(MODULE_ONE_ID);
+        Module testModule2 = moduleLoader.loadModule(MODULE_TWO_ID);
+        ModuleClassLoader classLoader1 = testModule1.getClassLoader();
+        ModuleClassLoader classLoader2 = testModule2.getClassLoader();
+
+        try {
+            Class<?> testClass1 = classLoader1.loadClass("org.jboss.modules.test.TestClass");
+            assertNotNull(testClass1);
+            Class<?> testClass2 = classLoader2.loadClass("org.jboss.modules.test.TestClass");
+            assertNotNull(testClass2);
+            assertNotNull(testClass1.getResource("/test.txt"));
+            assertNotNull(testClass2.getResource("/test.txt"));
+        } catch (ClassNotFoundException e) {
+            fail();
+        }
+        // cleanup
+        for (final ResourceLoader rl : classLoader1.getResourceLoaders()) { ((CloseAwareResourceLoader)rl).close(); }
+        for (final ResourceLoader rl : classLoader2.getResourceLoaders()) { ((CloseAwareResourceLoader)rl).close(); }
+        moduleLoader.unloadModuleLocal(testModule1);
+        moduleLoader.unloadModuleLocal(testModule2);
+
+        // SECOND PHASE
+        configureModules();
+        testModule1 = moduleLoader.loadModule(MODULE_ONE_ID);
+        moduleLoader.relink(testModule1);
+        testModule2 = moduleLoader.loadModule(MODULE_TWO_ID);
+        moduleLoader.relink(testModule2);
+        classLoader1 = testModule1.getClassLoader();
+        classLoader2 = testModule2.getClassLoader();
+
+        try {
+            Class<?> testClass1 = classLoader1.loadClass("org.jboss.modules.test.TestClass");
+            assertNotNull(testClass1);
+            Class<?> testClass2 = classLoader2.loadClass("org.jboss.modules.test.TestClass");
+            assertNotNull(testClass2);
+            assertNotNull(testClass1.getResource("/test.txt"));
+            assertNotNull(testClass2.getResource("/test.txt"));
+        } catch (ClassNotFoundException e) {
+            fail();
+        }
+        // cleanup
+        for (final ResourceLoader rl : classLoader1.getResourceLoaders()) { ((CloseAwareResourceLoader)rl).close(); }
+        for (final ResourceLoader rl : classLoader2.getResourceLoaders()) { ((CloseAwareResourceLoader)rl).close(); }
+        moduleLoader.unloadModuleLocal(testModule1);
+        moduleLoader.unloadModuleLocal(testModule2);
+    }
+
+}
diff --git a/src/test/java/org/jboss/modules/ModuleClassLoaderAliasTest.java b/src/test/java/org/jboss/modules/ModuleClassLoaderAliasTest.java
@@ -0,0 +1,103 @@
+package org.jboss.modules;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.net.URL;
+import java.nio.charset.Charset;
+
+import org.jboss.modules.util.Util;
+import org.junit.Test;
+
+/**
+ * Tests the basic functionality of alias modules.
+ *
+ * @author <a href="mailto:ropalka@redhat.com">Richard Opalka</a>
+ * @author <a href="https://github.com/ppalaga">Peter Palaga</a>
+ */
+public class ModuleClassLoaderAliasTest extends AbstractModuleTestCase {
+
+    @Test
+    public void testAliasCyclic() throws Exception {
+        final File repoRoot = getResource("test/repo");
+        final ModuleLoader moduleLoader = new LocalModuleLoader(new File[] { repoRoot });
+
+        Module testModule1 = moduleLoader.loadModule(ModuleIdentifier.fromString("test.alias-cyclic.module-one"));
+        Module testModule2 = moduleLoader.loadModule(ModuleIdentifier.fromString("test.alias-cyclic.module-two"));
+        ModuleClassLoader classLoader1 = testModule1.getClassLoader();
+        ModuleClassLoader classLoader2 = testModule2.getClassLoader();
+
+        try {
+            Class<?> testClass1 = classLoader1.loadClass("org.jboss.modules.test.ClassA");
+            assertNotNull(testClass1);
+            Class<?> testClass2 = classLoader2.loadClass("org.jboss.modules.test.ClassB");
+            assertNotNull(testClass2);
+
+            Class<?> testClass1ViaLoader2 = classLoader2.loadClass("org.jboss.modules.test.ClassA");
+            assertNotNull(testClass1ViaLoader2);
+            assertSame(testClass1, testClass1ViaLoader2);
+
+            Class<?> testClass2ViaLoader1 = classLoader1.loadClass("org.jboss.modules.test.ClassB");
+            assertNotNull(testClass2ViaLoader1);
+            assertSame(testClass2, testClass2ViaLoader1);
+
+            assertResourceString(testClass1.getResource("/test.txt"), "Test file 1");
+            assertResourceString(testClass2.getResource("/test.txt"), "Test file 2");
+
+            assertResourceString(classLoader1.getResource("/test.txt"), "Test file 1");
+            assertResourceString(classLoader2.getResource("/test.txt"), "Test file 2");
+
+        } catch (ClassNotFoundException e) {
+            fail();
+        }
+        moduleLoader.unloadModuleLocal(testModule1);
+        moduleLoader.unloadModuleLocal(testModule2);
+
+    }
+
+    @Test
+    public void testAliasSimple() throws Exception {
+        final File repoRoot = getResource("test/repo");
+        final ModuleLoader moduleLoader = new LocalModuleLoader(new File[] { repoRoot });
+
+        Module testModule1 = moduleLoader.loadModule(ModuleIdentifier.fromString("test.alias-simple.module-one"));
+        Module testModule2 = moduleLoader.loadModule(ModuleIdentifier.fromString("test.alias-simple.module-two"));
+        ModuleClassLoader classLoader1 = testModule1.getClassLoader();
+        ModuleClassLoader classLoader2 = testModule2.getClassLoader();
+
+        Class<?> testClass1 = classLoader1.loadClass("org.jboss.modules.test.ClassA");
+        assertNotNull(testClass1);
+        Class<?> testClass2 = classLoader2.loadClass("org.jboss.modules.test.ClassB");
+        assertNotNull(testClass2);
+
+        Class<?> testClass2ViaLoader1 = classLoader1.loadClass("org.jboss.modules.test.ClassB");
+        assertNotNull(testClass2ViaLoader1);
+        assertSame(testClass2, testClass2ViaLoader1);
+
+        try {
+            classLoader2.loadClass("org.jboss.modules.test.ClassA");
+            fail("ClassNotFoundException expected");
+        } catch (ClassNotFoundException expected) {
+        }
+
+        assertResourceString(testClass1.getResource("/test.txt"), "Test file 1");
+        assertResourceString(testClass2.getResource("/test.txt"), "Test file 2");
+
+        assertResourceString(classLoader1.getResource("/test.txt"), "Test file 1");
+        assertResourceString(classLoader2.getResource("/test.txt"), "Test file 2");
+
+        moduleLoader.unloadModuleLocal(testModule1);
+        moduleLoader.unloadModuleLocal(testModule2);
+
+    }
+
+    private static void assertResourceString(URL resource, String expected) throws IOException {
+        assertNotNull(resource);
+        byte[] bytes = Util.readBytes(resource.openStream());
+        assertEquals(expected, new String(bytes, Charset.forName("utf-8")));
+    }
+}
diff --git a/src/test/resources/test/aliasmodule/rootOne/test.txt b/src/test/resources/test/aliasmodule/rootOne/test.txt
@@ -0,0 +1 @@
+Test file 1
diff --git a/src/test/resources/test/aliasmodule/rootTwo/test.txt b/src/test/resources/test/aliasmodule/rootTwo/test.txt
@@ -0,0 +1 @@
+Test file 2
diff --git a/...urces/test/repo/test/alias-cyclic/module-one/main/dir/org/jboss/modules/test/ClassA.class b/...urces/test/repo/test/alias-cyclic/module-one/main/dir/org/jboss/modules/test/ClassA.class
diff --git a/src/test/resources/test/repo/test/alias-cyclic/module-one/main/dir/test.txt b/src/test/resources/test/repo/test/alias-cyclic/module-one/main/dir/test.txt
@@ -0,0 +1 @@
+Test file 1