diff --git a/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecProcessor.java b/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecProcessor.java index 9e08d12b02d..bec3cacf4a0 100644 --- a/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecProcessor.java +++ b/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecProcessor.java @@ -9,11 +9,13 @@ import java.security.Permission; import java.security.Permissions; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; import java.util.List; import java.util.PropertyPermission; +import java.util.Set; import java.util.function.Consumer; import org.jboss.as.server.deployment.Attachments; @@ -146,9 +148,9 @@ private void deployModuleSpec(final DeploymentPhaseContext phaseContext) throws * @param module The additional module */ private void addAllDependenciesAndPermissions(final ModuleSpecification moduleSpecification, final AdditionalModuleSpecification module) { - module.addSystemDependencies(moduleSpecification.getSystemDependencies()); - module.addLocalDependencies(moduleSpecification.getLocalDependencies()); - for(ModuleDependency dep : moduleSpecification.getUserDependencies()) { + module.addSystemDependencies(moduleSpecification.getSystemDependenciesSet()); + module.addLocalDependencies(moduleSpecification.getLocalDependenciesSet()); + for(ModuleDependency dep : moduleSpecification.getUserDependenciesSet()) { if(!dep.getIdentifier().equals(module.getModuleIdentifier())) { module.addUserDependency(dep); } @@ -202,9 +204,9 @@ private ServiceName createModuleService(final DeploymentPhaseContext phaseContex for (final DependencySpec dep : moduleSpecification.getModuleSystemDependencies()) { specBuilder.addDependency(dep); } - final List dependencies = moduleSpecification.getSystemDependencies(); - final List localDependencies = moduleSpecification.getLocalDependencies(); - final List userDependencies = moduleSpecification.getUserDependencies(); + final Set dependencies = moduleSpecification.getSystemDependenciesSet(); + final Set localDependencies = moduleSpecification.getLocalDependenciesSet(); + final Set userDependencies = moduleSpecification.getUserDependenciesSet(); final List permFactories = moduleSpecification.getPermissionFactories(); @@ -301,7 +303,7 @@ private void installAliases(final ModuleSpecification moduleSpecification, final } } - private void createDependencies(final ModuleSpec.Builder specBuilder, final List apiDependencies, final boolean requireTransitive) { + private void createDependencies(final ModuleSpec.Builder specBuilder, final Collection apiDependencies, final boolean requireTransitive) { if (apiDependencies != null) { for (final ModuleDependency dependency : apiDependencies) { final boolean export = requireTransitive ? true : dependency.isExport(); diff --git a/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java b/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java index 48eda430258..aece105b659 100644 --- a/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java +++ b/server/src/main/java/org/jboss/as/server/deployment/module/ModuleSpecification.java @@ -14,6 +14,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.Predicate; import org.jboss.as.server.deployment.SimpleAttachable; import org.jboss.modules.DependencySpec; @@ -24,6 +25,10 @@ /** * Information used to build a module. * + * This class is not thread safe. It should only be used by the deployment unit processors + * associated with a single deployment unit, with a parent deployment and a subdeployment considered to + * be separate deployments. + * * @author Stuart Douglas * @author Marius Bogoevici */ @@ -32,15 +37,19 @@ public class ModuleSpecification extends SimpleAttachable { /** * System dependencies are dependencies that are added automatically by the container. */ - private final List systemDependencies = new ArrayList<>(); + private final Set systemDependenciesSet = new LinkedHashSet<>(); - /** Unique names of system dependencies, used to ensure only one system ModuleDependency per name is added. */ - private final Set systemDependenciesSet = new HashSet<>(); + /** + * List view of {@link #systemDependenciesSet}. + */ + @SuppressWarnings("DeprecatedIsStillUsed") + @Deprecated(forRemoval = true) + private final List systemDependencies = new ArrayList<>(); /** - * Local dependencies are dependencies on other parts of the deployment, such as class-path entry + * Local dependencies are dependencies on other parts of the deployment, such as a class-path entry */ - private final Set localDependencies = new LinkedHashSet<>(); + private final Set localDependenciesSet = new LinkedHashSet<>(); /** * If set to true this indicates that a dependency on this module requires a dependency on all it's local @@ -54,11 +63,14 @@ public class ModuleSpecification extends SimpleAttachable { *

* User dependencies are not affected by exclusions. */ - private final List userDependencies = new ArrayList<>(); + private final Set userDependenciesSet = new HashSet<>(); + /** - * Set view of user dependencies, used to prevent duplicates in the userDependencies list. + * List view of {@link #userDependenciesSet}. */ - private final Set userDependenciesSet = new HashSet<>(); + @SuppressWarnings("DeprecatedIsStillUsed") + @Deprecated(forRemoval = true) + private final List userDependencies = new ArrayList<>(); private final List resourceLoaders = new ArrayList<>(); @@ -122,7 +134,7 @@ public class ModuleSpecification extends SimpleAttachable { * JBoss modules system dependencies, which allow you to specify dependencies on the app class loader * to get access to JDK classes. */ - private final List moduleSystemDependencies = new ArrayList(); + private final List moduleSystemDependencies = new ArrayList<>(); /** * The minimum permission set for this module, wrapped as {@code PermissionFactory} instances. @@ -130,57 +142,84 @@ public class ModuleSpecification extends SimpleAttachable { private final List permissionFactories = new ArrayList<>(); public void addSystemDependency(final ModuleDependency dependency) { - if (!exclusions.contains(dependency.getIdentifier()) && !systemDependenciesSet.contains(dependency.getIdentifier())) { - this.systemDependencies.add(dependency); - this.systemDependenciesSet.add(dependency.getIdentifier()); + if (!exclusions.contains(dependency.getIdentifier())) { + if (systemDependenciesSet.add(dependency)) { + resetDependencyLists(this.systemDependencies); + } } else { excludedDependencies.add(dependency.getIdentifier()); } } public void addSystemDependencies(final Collection dependencies) { - allDependencies = null; for (final ModuleDependency dependency : dependencies) { addSystemDependency(dependency); } } public void addUserDependency(final ModuleDependency dependency) { - allDependencies = null; if (this.userDependenciesSet.add(dependency)) { - this.userDependencies.add(dependency); + resetDependencyLists(this.userDependencies); } } public void addUserDependencies(final Collection dependencies) { - allDependencies = null; for (final ModuleDependency dependency : dependencies) { addUserDependency(dependency); } } + /** + * Remove user dependencies that match the predicate. + * + * @param predicate test for whether a dependency should be removed. Cannot be {@code null}. + */ + public void removeUserDependencies(final Predicate predicate) { + Iterator iter = userDependenciesSet.iterator(); + while (iter.hasNext()) { + ModuleDependency md = iter.next(); + if (predicate.test(md)) { + iter.remove(); + resetDependencyLists(userDependencies); + } + } + } + public void addLocalDependency(final ModuleDependency dependency) { - allDependencies = null; if (!exclusions.contains(dependency.getIdentifier())) { - this.localDependencies.add(dependency); + if (this.localDependenciesSet.add(dependency)) { + resetDependencyLists(null); + } } else { excludedDependencies.add(dependency.getIdentifier()); } } public void addLocalDependencies(final Collection dependencies) { - allDependencies = null; for (final ModuleDependency dependency : dependencies) { addLocalDependency(dependency); } } + /** @deprecated use {@link #getSystemDependenciesSet()} */ + @Deprecated(forRemoval = true) public List getSystemDependencies() { + if (systemDependencies.isEmpty()) { + systemDependencies.addAll(systemDependenciesSet); + } return Collections.unmodifiableList(systemDependencies); } + /** + * System dependencies are dependencies that are added automatically by the container. + * + * @return system dependencies iterable in order of addition. Will not return {@code null}. + */ + public Set getSystemDependenciesSet() { + return Collections.unmodifiableSet(systemDependenciesSet); + } + public void addExclusion(final ModuleIdentifier exclusion) { - allDependencies = null; final String targetModule = ModuleAliasChecker.getTargetModule(exclusion.toString()); if (targetModule != null) { final ModuleIdentifier identifier = ModuleIdentifier.create(targetModule); @@ -200,18 +239,20 @@ public void addExclusion(final ModuleIdentifier exclusion) { } // list of exclusions, aliases or target modules exclusions.add(exclusion); - Iterator it = systemDependencies.iterator(); + Iterator it = systemDependenciesSet.iterator(); while (it.hasNext()) { final ModuleDependency dep = it.next(); if (dep.getIdentifier().equals(exclusion)) { it.remove(); + resetDependencyLists(this.systemDependencies); } } - it = localDependencies.iterator(); + it = localDependenciesSet.iterator(); while (it.hasNext()) { final ModuleDependency dep = it.next(); if (dep.getIdentifier().equals(exclusion)) { it.remove(); + resetDependencyLists(null); } } } @@ -223,24 +264,63 @@ public void addExclusions(final Iterable exclusions) { } + /** + * @deprecated use {@link #getLocalDependenciesSet()} ()} + */ + @Deprecated(forRemoval = true) public List getLocalDependencies() { - return Collections.unmodifiableList(new ArrayList<>(localDependencies)); + return Collections.unmodifiableList(new ArrayList<>(localDependenciesSet)); } + + /** + * Local dependencies are dependencies on other parts of the deployment, such as a class-path entry + * + * @return local dependencies iterable in order of addition. Will not return {@code null}. + */ + public Set getLocalDependenciesSet() { + return Collections.unmodifiableSet(localDependenciesSet); + } + + /** + * @deprecated use {@link #getUserDependenciesSet()} + */ + @SuppressWarnings("DeprecatedIsStillUsed") + @Deprecated(forRemoval = true) public List getUserDependencies() { + if (userDependencies.isEmpty()) { + userDependencies.addAll(userDependenciesSet); + } return Collections.unmodifiableList(userDependencies); } + /** + * User dependencies are dependencies that the user has specifically added, either via jboss-deployment-structure.xml + * or via the manifest. + *

+ * User dependencies are not affected by exclusions. + * + * @return user dependencies iterable in order of addition. Will not return {@code null}. + */ + public Set getUserDependenciesSet() { + return Collections.unmodifiableSet(userDependenciesSet); + } + /** * Gets a modifiable view of the user dependencies list. * * @return The user dependencies + * + * @deprecated use {@link #addUserDependency(ModuleDependency)} and {@link #removeUserDependencies(Predicate)} */ + @SuppressWarnings("DeprecatedIsStillUsed") + @Deprecated(forRemoval = true) public Collection getMutableUserDependencies() { - allDependencies = null; + resetDependencyLists(this.userDependencies); return userDependenciesSet; } + @SuppressWarnings("unused") public void addResourceLoader(final ResourceLoaderSpec resourceLoader) { this.resourceLoaders.add(resourceLoader); } @@ -285,32 +365,32 @@ public boolean isPublicModule() { return publicModule; } + @SuppressWarnings("unused") public void setPublicModule(boolean publicModule) { this.publicModule = publicModule; } /** - * Returns true if the {@link #localDependencies} added for this {@link ModuleSpecification} should be made + * Returns true if the {@link #getLocalDependenciesSet() local dependencies} added for this {@link ModuleSpecification} should be made * transitive (i.e. if any other module 'B' depends on the module 'A' represented by this {@link ModuleSpecification}, then * module 'B' will be added with all "local dependencies" that are applicable for module "A"). Else returns false. * - * @return - * @see {@link #localDependencies} + * @return {@code true} if local dependencies should be made transitive + * @see #getLocalDependenciesSet() */ public boolean isLocalDependenciesTransitive() { return localDependenciesTransitive; } /** - * Sets whether the {@link #localDependencies} applicable for this {@link ModuleSpecification} are to be treated as transitive dependencies + * Sets whether the {@link #getLocalDependenciesSet() local dependencies} applicable for this {@link ModuleSpecification} are to be treated as transitive dependencies * for modules which depend on the module represented by this {@link ModuleSpecification} * - * @param localDependenciesTransitive True if the {@link #localDependencies} added for this {@link ModuleSpecification} should be made + * @param localDependenciesTransitive {@code true} if the {@link #getLocalDependenciesSet()} added for this {@link ModuleSpecification} should be made * transitive (i.e. if any other module 'B' depends on the module 'A' represented by * this {@link ModuleSpecification}, then module 'B' will be added with * all "local dependencies" that are applicable for module "A"). False otherwise - * @return - * @see {@link #localDependencies} + * @see #getLocalDependenciesSet() */ public void setLocalDependenciesTransitive(final boolean localDependenciesTransitive) { this.localDependenciesTransitive = localDependenciesTransitive; @@ -324,6 +404,7 @@ public void setLocalLast(final boolean localLast) { this.localLast = localLast; } + @SuppressWarnings("unused") public void addAlias(final ModuleIdentifier moduleIdentifier) { aliases.add(moduleIdentifier); } @@ -338,10 +419,10 @@ public List getAliases() { public List getAllDependencies() { if (allDependencies == null) { - allDependencies = new ArrayList(); - allDependencies.addAll(systemDependencies); - allDependencies.addAll(userDependencies); - allDependencies.addAll(localDependencies); + allDependencies = new ArrayList<>(); + allDependencies.addAll(systemDependenciesSet); + allDependencies.addAll(userDependenciesSet); + allDependencies.addAll(localDependenciesSet); } return allDependencies; } @@ -390,4 +471,11 @@ public Set getNonexistentExcludedDependencies() { return unExcludedModuleExclusion; } + private void resetDependencyLists(List listView) { + if (listView != null) { + listView.clear(); + } + allDependencies = null; + } + } diff --git a/server/src/main/java/org/jboss/as/server/moduleservice/ModuleLoadService.java b/server/src/main/java/org/jboss/as/server/moduleservice/ModuleLoadService.java index ad7a6b66673..75ec2772b63 100644 --- a/server/src/main/java/org/jboss/as/server/moduleservice/ModuleLoadService.java +++ b/server/src/main/java/org/jboss/as/server/moduleservice/ModuleLoadService.java @@ -5,6 +5,7 @@ package org.jboss.as.server.moduleservice; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; @@ -36,13 +37,13 @@ public class ModuleLoadService implements Service { private final InjectedValue serviceModuleLoader = new InjectedValue(); private final InjectedValue moduleDefinitionInjectedValue = new InjectedValue(); private final List allDependencies; - private final List systemDependencies; - private final List userDependencies; - private final List localDependencies; + private final Collection systemDependencies; + private final Collection userDependencies; + private final Collection localDependencies; private volatile Module module; - private ModuleLoadService(final List systemDependencies, final List localDependencies, final List userDependencies) { + private ModuleLoadService(final Collection systemDependencies, final Collection localDependencies, final Collection userDependencies) { this.systemDependencies = systemDependencies; this.localDependencies = localDependencies; this.userDependencies = userDependencies; @@ -129,7 +130,7 @@ public static ServiceName install(final ServiceTarget target, final ModuleIdenti return install(target, identifier, service); } - public static ServiceName install(final ServiceTarget target, final ModuleIdentifier identifier, final List systemDependencies, final List localDependencies, final List userDependencies) { + public static ServiceName install(final ServiceTarget target, final ModuleIdentifier identifier, final Collection systemDependencies, final Collection localDependencies, final Collection userDependencies) { final ModuleLoadService service = new ModuleLoadService(systemDependencies, localDependencies, userDependencies); return install(target, identifier, service); } diff --git a/server/src/test/java/org/jboss/as/server/deployment/module/ModuleSpecificationTestCase.java b/server/src/test/java/org/jboss/as/server/deployment/module/ModuleSpecificationTestCase.java new file mode 100644 index 00000000000..b0bae962251 --- /dev/null +++ b/server/src/test/java/org/jboss/as/server/deployment/module/ModuleSpecificationTestCase.java @@ -0,0 +1,80 @@ +/* + * Copyright The WildFly Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.jboss.as.server.deployment.module; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.Collection; +import java.util.List; +import java.util.Set; + +import org.jboss.modules.ModuleLoader; +import org.junit.Test; + +public class ModuleSpecificationTestCase { + + private static final ModuleLoader TEST_LOADER = new ModuleLoader(ModuleLoader.NO_FINDERS); + private static final ModuleDependency DEP_A = createModuleDependency("a"); + private static final ModuleDependency DEP_B = createModuleDependency("b"); + private static final ModuleDependency DEP_C = createModuleDependency("c"); + private static final Set ALL_USER_DEPS = Set.of(DEP_A, DEP_B, DEP_C); + + private static ModuleDependency createModuleDependency(String identifier) { + return new ModuleDependency(TEST_LOADER, identifier, false, false, true, false); + } + + @Test + public void testUserDependencyConsistency() { + ModuleSpecification ms = new ModuleSpecification(); + ms.addUserDependencies(ALL_USER_DEPS); + + // Sanity check + assertEquals(ALL_USER_DEPS, ms.getUserDependenciesSet()); + List userDepList = ms.getUserDependencies(); + assertEquals(ALL_USER_DEPS.size(), userDepList.size()); + for (ModuleDependency md : ALL_USER_DEPS) { + assertTrue(userDepList.contains(md)); + } + + // Removal consistency + ms.removeUserDependencies(md -> md.getIdentifier().getName().equals("a")); + userDepList = ms.getUserDependencies(); + Set userDepSet = ms.getUserDependenciesSet(); + assertEquals(ALL_USER_DEPS.size() -1, userDepList.size()); + assertEquals(ALL_USER_DEPS.size() -1, userDepSet.size()); + for (ModuleDependency md : ALL_USER_DEPS) { + boolean shouldFind = !md.equals(DEP_A); + assertEquals(shouldFind, userDepList.contains(md)); + assertEquals(shouldFind, userDepSet.contains(md)); + } + + // Deprecated removal consistency + ms.addUserDependency(DEP_A); + Collection modifiable = ms.getMutableUserDependencies(); + assertTrue(modifiable.remove(DEP_A)); + userDepList = ms.getUserDependencies(); + userDepSet = ms.getUserDependenciesSet(); + assertEquals(ALL_USER_DEPS.size() -1, userDepList.size()); + assertEquals(ALL_USER_DEPS.size() -1, userDepSet.size()); + for (ModuleDependency md : ALL_USER_DEPS) { + boolean shouldFind = !md.equals(DEP_A); + assertEquals(shouldFind, userDepList.contains(md)); + assertEquals(shouldFind, userDepSet.contains(md)); + } + + // Demonstrate why getMutableUserDependencies() is deprecated + // It hands out a ref to mutable internal state to external code + // which can modify it at some later point, unknown to the ModuleSpecification + modifiable.remove(DEP_B); + userDepList = ms.getUserDependencies(); + userDepSet = ms.getUserDependenciesSet(); + assertEquals(userDepList.size(), userDepSet.size() + 1); + assertTrue(userDepList.contains(DEP_B)); + assertFalse(userDepSet.contains(DEP_B)); + } +}