Fix nasty concurrency issue in AbstractSession (#1479)

* Register the session explicitely * Fix thread interrupted
apache · Apr 23, 2024 · 52c5659 · 52c5659
1 parent aae74df
commit 52c5659
Show file tree

Hide file tree

Showing 14 changed files with 71 additions and 45 deletions.
diff --git a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/AbstractSession.java b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/AbstractSession.java
@@ -94,7 +94,6 @@ public AbstractSession(
         this.repositorySystem = repositorySystem;
         this.repositories = getRepositories(repositories, resolverRepositories);
         this.lookup = lookup;
-        this.session.getData().set(InternalSession.class, this);
     }
 
     @Override
@@ -155,13 +154,13 @@ public List<org.eclipse.aether.graph.Dependency> toDependencies(
         return dependencies == null ? null : map(dependencies, d -> toDependency(d, managed));
     }
 
-    protected List<RemoteRepository> getRepositories(
+    static List<RemoteRepository> getRepositories(
             List<RemoteRepository> repositories,
             List<org.eclipse.aether.repository.RemoteRepository> resolverRepositories) {
         if (repositories != null) {
             return repositories;
         } else if (resolverRepositories != null) {
-            return map(resolverRepositories, this::getRemoteRepository);
+            return map(resolverRepositories, DefaultRemoteRepository::new);
         } else {
             throw new IllegalArgumentException("no remote repositories provided");
         }

diff --git a/maven-api-impl/src/main/java/org/apache/maven/internal/impl/InternalSession.java b/maven-api-impl/src/main/java/org/apache/maven/internal/impl/InternalSession.java
@@ -45,6 +45,12 @@ static InternalSession from(org.eclipse.aether.RepositorySystemSession session)
         return cast(InternalSession.class, session.getData().get(InternalSession.class), "session");
     }
 
+    static void associate(org.eclipse.aether.RepositorySystemSession rsession, Session session) {
+        if (!rsession.getData().set(InternalSession.class, null, from(session))) {
+            throw new IllegalStateException("A maven session is already associated with the repository session");
+        }
+    }
+
     RemoteRepository getRemoteRepository(org.eclipse.aether.repository.RemoteRepository repository);
 
     Node getNode(org.eclipse.aether.graph.DependencyNode node);

diff --git a/maven-api-impl/src/test/java/org/apache/maven/internal/impl/standalone/ApiRunner.java b/maven-api-impl/src/test/java/org/apache/maven/internal/impl/standalone/ApiRunner.java
@@ -414,6 +414,7 @@ static Session newSession(RepositorySystem system, Lookup lookup) {
                 .map(repositoryFactory::createRemote)
                 .toList();
         InternalSession s = (InternalSession) session.withRemoteRepositories(repositories);
+        InternalSession.associate(rsession, s);
         return s;
 
         // List<RemoteRepository> repositories = repositoryFactory.createRemote();

diff --git a/maven-compat/src/test/java/org/apache/maven/AbstractCoreMavenComponentTestCase.java b/maven-compat/src/test/java/org/apache/maven/AbstractCoreMavenComponentTestCase.java
@@ -35,6 +35,7 @@
 import org.apache.maven.execution.MavenExecutionRequest;
 import org.apache.maven.execution.MavenSession;
 import org.apache.maven.internal.impl.DefaultSession;
+import org.apache.maven.internal.impl.InternalSession;
 import org.apache.maven.model.Build;
 import org.apache.maven.model.Dependency;
 import org.apache.maven.model.Exclusion;
@@ -128,6 +129,7 @@ protected MavenSession createMavenSession(File pom, Properties executionProperti
                 getContainer(), configuration.getRepositorySession(), request, new DefaultMavenExecutionResult());
         DefaultSession iSession =
                 new DefaultSession(session, mock(org.eclipse.aether.RepositorySystem.class), null, null, null, null);
+        InternalSession.associate(session.getRepositorySession(), iSession);
         session.setSession(iSession);
 
         List<MavenProject> projects = new ArrayList<>();

diff --git a/maven-compat/src/test/java/org/apache/maven/project/AbstractMavenProjectTestCase.java b/maven-compat/src/test/java/org/apache/maven/project/AbstractMavenProjectTestCase.java
@@ -33,6 +33,7 @@
 import org.apache.maven.execution.MavenSession;
 import org.apache.maven.internal.impl.DefaultLookup;
 import org.apache.maven.internal.impl.DefaultSession;
+import org.apache.maven.internal.impl.InternalSession;
 import org.apache.maven.model.building.ModelBuildingException;
 import org.apache.maven.model.building.ModelProblem;
 import org.apache.maven.repository.RepositorySystem;
@@ -153,12 +154,13 @@ protected void initRepoSession(ProjectBuildingRequest request) {
         DefaultMavenExecutionRequest mavenExecutionRequest = new DefaultMavenExecutionRequest();
         MavenSession msession =
                 new MavenSession(getContainer(), session, mavenExecutionRequest, new DefaultMavenExecutionResult());
-        new DefaultSession(
+        DefaultSession iSession = new DefaultSession(
                 msession,
                 mock(org.eclipse.aether.RepositorySystem.class),
                 null,
                 null,
                 new DefaultLookup(container),
                 null);
+        InternalSession.associate(session, iSession);
     }
 }
diff --git a/maven-compat/src/test/java/org/apache/maven/repository/LegacyRepositorySystemTest.java b/maven-compat/src/test/java/org/apache/maven/repository/LegacyRepositorySystemTest.java
@@ -122,6 +122,7 @@ void testThatASystemScopedDependencyIsNotResolvedFromRepositories() throws Excep
                 new MavenSession(container, session, mavenExecutionRequest, new DefaultMavenExecutionResult());
         legacySupport.setSession(mavenSession);
         InternalSession iSession = new DefaultSession(mavenSession, null, null, null, null, null);
+        InternalSession.associate(session, iSession);
 
         ArtifactResolutionResult result = repositorySystem.resolve(request);
         resolutionErrorHandler.throwErrors(request, result);

diff --git a/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSession.java b/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSession.java
@@ -73,6 +73,9 @@ public DefaultSession(
     }
 
     public MavenSession getMavenSession() {
+        if (mavenSession == null) {
+            throw new IllegalArgumentException("Found null mavenSession on session " + this);
+        }
         return mavenSession;
     }
 
@@ -94,19 +97,19 @@ public List<ArtifactRepository> toArtifactRepositories(List<RemoteRepository> re
     @Nonnull
     @Override
     public Settings getSettings() {
-        return mavenSession.getSettings().getDelegate();
+        return getMavenSession().getSettings().getDelegate();
     }
 
     @Nonnull
     @Override
     public Map<String, String> getUserProperties() {
-        return Collections.unmodifiableMap(new PropertiesAsMap(mavenSession.getUserProperties()));
+        return Collections.unmodifiableMap(new PropertiesAsMap(getMavenSession().getUserProperties()));
     }
 
     @Nonnull
     @Override
     public Map<String, String> getSystemProperties() {
-        return Collections.unmodifiableMap(new PropertiesAsMap(mavenSession.getSystemProperties()));
+        return Collections.unmodifiableMap(new PropertiesAsMap(getMavenSession().getSystemProperties()));
     }
 
     @Nonnull
@@ -128,29 +131,29 @@ public Version getMavenVersion() {
 
     @Override
     public int getDegreeOfConcurrency() {
-        return mavenSession.getRequest().getDegreeOfConcurrency();
+        return getMavenSession().getRequest().getDegreeOfConcurrency();
     }
 
     @Nonnull
     @Override
     public Instant getStartTime() {
-        return mavenSession.getRequest().getStartTime().toInstant();
+        return getMavenSession().getRequest().getStartTime().toInstant();
     }
 
     @Override
     public Path getRootDirectory() {
-        return mavenSession.getRequest().getRootDirectory();
+        return getMavenSession().getRequest().getRootDirectory();
     }
 
     @Override
     public Path getTopDirectory() {
-        return mavenSession.getRequest().getTopDirectory();
+        return getMavenSession().getRequest().getTopDirectory();
     }
 
     @Nonnull
     @Override
     public List<Project> getProjects() {
-        return getProjects(mavenSession.getProjects());
+        return getProjects(getMavenSession().getProjects());
     }
 
     @Nonnull
@@ -161,14 +164,14 @@ public Map<String, Object> getPluginContext(Project project) {
             MojoExecution mojoExecution = lookup.lookup(MojoExecution.class);
             MojoDescriptor mojoDescriptor = mojoExecution.getMojoDescriptor();
             PluginDescriptor pluginDescriptor = mojoDescriptor.getPluginDescriptor();
-            return mavenSession.getPluginContext(pluginDescriptor, ((DefaultProject) project).getProject());
+            return getMavenSession().getPluginContext(pluginDescriptor, ((DefaultProject) project).getProject());
         } catch (LookupException e) {
             throw new MavenException("The PluginContext is only available during a mojo execution", e);
         }
     }
 
     protected Session newSession(RepositorySystemSession repoSession, List<RemoteRepository> repositories) {
-        final MavenSession ms = nonNull(mavenSession);
+        final MavenSession ms = nonNull(getMavenSession());
         final MavenSession mss;
         if (repoSession != ms.getRepositorySession()) {
             mss = new MavenSession(repoSession, ms.getRequest(), ms.getResult());

diff --git a/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSessionFactory.java b/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSessionFactory.java
@@ -50,7 +50,9 @@ public DefaultSessionFactory(
     }
 
     public InternalSession newSession(MavenSession mavenSession) {
-        return new DefaultSession(
+        InternalSession session = new DefaultSession(
                 mavenSession, repositorySystem, null, mavenRepositorySystem, lookup, runtimeInformation);
+        InternalSession.associate(mavenSession.getRepositorySession(), session);
+        return session;
     }
 }
diff --git a/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java b/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
@@ -28,6 +28,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.ForkJoinTask;
+import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.maven.api.model.Dependency;
@@ -85,7 +86,7 @@ public class ProjectModelResolver implements ModelResolver {
 
     private final ProjectBuildingRequest.RepositoryMerging repositoryMerging;
 
-    private final Map<String, ForkJoinTask<Result>> parentCache;
+    private final Map<String, Future<Result>> parentCache;
 
     public ProjectModelResolver(
             RepositorySystemSession session,
@@ -189,38 +190,39 @@ public ModelSource resolveModel(final Parent parent, AtomicReference<Parent> mod
             throws UnresolvableModelException {
         Result result;
         try {
-            ForkJoinTask<Result> future = parentCache.computeIfAbsent(parent.getId(), id -> new ForkJoinTask<>() {
-                Result result;
-
-                @Override
-                public Result getRawResult() {
-                    return result;
-                }
+            Future<Result> future = parentCache.computeIfAbsent(parent.getId(), id -> {
+                ForkJoinPool pool = new ForkJoinPool(MAX_CAP);
+                ForkJoinTask<Result> task = new ForkJoinTask<>() {
+                    Result result;
+
+                    @Override
+                    public Result getRawResult() {
+                        return result;
+                    }
 
-                @Override
-                protected void setRawResult(Result result) {
-                    this.result = result;
-                }
+                    @Override
+                    protected void setRawResult(Result result) {
+                        this.result = result;
+                    }
 
-                @Override
-                protected boolean exec() {
-                    try {
-                        AtomicReference<Parent> modified = new AtomicReference<>();
-                        ModelSource source = doResolveModel(parent, modified);
-                        result = new Result(source, modified.get(), null);
-                    } catch (Exception e) {
-                        result = new Result(null, null, e);
+                    @Override
+                    protected boolean exec() {
+                        try {
+                            AtomicReference<Parent> modified = new AtomicReference<>();
+                            ModelSource source = doResolveModel(parent, modified);
+                            result = new Result(source, modified.get(), null);
+                        } catch (Exception e) {
+                            result = new Result(null, null, e);
+                        } finally {
+                            pool.shutdown();
+                        }
+                        return true;
                     }
-                    return true;
-                }
+                };
+                pool.submit(task);
+                return task;
             });
-            ForkJoinPool pool = new ForkJoinPool(MAX_CAP);
-            try {
-                pool.execute(future);
-                result = future.get();
-            } finally {
-                pool.shutdownNow();
-            }
+            result = future.get();
         } catch (Exception e) {
             throw new UnresolvableModelException(e, parent.getGroupId(), parent.getArtifactId(), parent.getVersion());
         }

diff --git a/maven-core/src/test/java/org/apache/maven/MavenTestHelper.java b/maven-core/src/test/java/org/apache/maven/MavenTestHelper.java
@@ -23,6 +23,7 @@
 import org.apache.maven.execution.DefaultMavenExecutionResult;
 import org.apache.maven.execution.MavenSession;
 import org.apache.maven.internal.impl.DefaultSession;
+import org.apache.maven.internal.impl.InternalSession;
 import org.eclipse.aether.DefaultRepositorySystemSession;
 
 public class MavenTestHelper {
@@ -31,6 +32,7 @@ public static DefaultRepositorySystemSession createSession(MavenRepositorySystem
         DefaultMavenExecutionRequest request = new DefaultMavenExecutionRequest();
         MavenSession mavenSession = new MavenSession(repoSession, request, new DefaultMavenExecutionResult());
         DefaultSession session = new DefaultSession(mavenSession, null, null, repositorySystem, null, null);
+        InternalSession.associate(repoSession, session);
         return repoSession;
     }
 }
diff --git a/maven-core/src/test/java/org/apache/maven/internal/impl/TestApi.java b/maven-core/src/test/java/org/apache/maven/internal/impl/TestApi.java
@@ -123,6 +123,7 @@ void setup() {
                 new RemoteRepository.Builder("mirror", "default", "file:target/test-classes/repo").build());
         this.session = session.withLocalRepository(localRepository)
                 .withRemoteRepositories(Collections.singletonList(remoteRepository));
+        InternalSession.associate(rss, this.session);
         sessionScope.enter();
         sessionScope.seed(InternalMavenSession.class, InternalMavenSession.from(this.session));
     }

diff --git a/...re/src/test/java/org/apache/maven/internal/transformation/AbstractRepositoryTestCase.java b/...re/src/test/java/org/apache/maven/internal/transformation/AbstractRepositoryTestCase.java
@@ -26,6 +26,7 @@
 import org.apache.maven.execution.DefaultMavenExecutionResult;
 import org.apache.maven.execution.MavenSession;
 import org.apache.maven.internal.impl.DefaultSession;
+import org.apache.maven.internal.impl.InternalSession;
 import org.codehaus.plexus.PlexusContainer;
 import org.codehaus.plexus.testing.PlexusTest;
 import org.eclipse.aether.DefaultRepositorySystemSession;
@@ -71,6 +72,7 @@ public static RepositorySystemSession newMavenRepositorySystemSession(Repository
         DefaultMavenExecutionRequest request = new DefaultMavenExecutionRequest();
         MavenSession mavenSession = new MavenSession(rsession, request, new DefaultMavenExecutionResult());
         DefaultSession session = new DefaultSession(mavenSession, null, null, null, null, null);
+        InternalSession.associate(rsession, session);
 
         return rsession;
     }

diff --git a/maven-core/src/test/java/org/apache/maven/model/ModelBuilderTest.java b/maven-core/src/test/java/org/apache/maven/model/ModelBuilderTest.java
@@ -70,6 +70,7 @@ void testModelBuilder() throws Exception {
         MavenSession msession = new MavenSession(rsession, mavenRequest, new DefaultMavenExecutionResult());
         InternalSession session =
                 new DefaultSession(msession, repositorySystem, null, mavenRepositorySystem, null, null);
+        InternalSession.associate(rsession, session);
 
         List<ProjectBuildingResult> results = projectBuilder.build(
                 Collections.singletonList(new File("src/test/resources/projects/tree/pom.xml")), true, request);

diff --git a/...n-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java b/...n-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java
@@ -53,6 +53,7 @@
 import org.apache.maven.internal.impl.DefaultSession;
 import org.apache.maven.internal.impl.DefaultVersionParser;
 import org.apache.maven.internal.impl.DefaultVersionRangeResolver;
+import org.apache.maven.internal.impl.InternalSession;
 import org.apache.maven.plugin.PluginResolutionException;
 import org.apache.maven.plugin.internal.DefaultPluginDependenciesResolver;
 import org.apache.maven.resolver.MavenChainedWorkspaceReader;
@@ -130,7 +131,8 @@ public List<CoreExtensionEntry> loadCoreExtensions(
                 .setWorkspaceReader(new MavenChainedWorkspaceReader(request.getWorkspaceReader(), ideWorkspaceReader))
                 .build()) {
             MavenSession mSession = new MavenSession(repoSession, request, new DefaultMavenExecutionResult());
-            new SimpleSession(mSession, repoSystem, null);
+            InternalSession iSession = new SimpleSession(mSession, repoSystem, null);
+            InternalSession.associate(repoSession, iSession);
 
             List<RemoteRepository> repositories = RepositoryUtils.toRepos(request.getPluginArtifactRepositories());
             Interpolator interpolator = createInterpolator(request);