From 52c5659b25f9153091db5356dfa50273e50acf9b Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Apr 2024 14:55:57 +0200 Subject: [PATCH] Fix nasty concurrency issue in AbstractSession (#1479) * Register the session explicitely * Fix thread interrupted --- .../maven/internal/impl/AbstractSession.java | 5 +- .../maven/internal/impl/InternalSession.java | 6 ++ .../internal/impl/standalone/ApiRunner.java | 1 + .../AbstractCoreMavenComponentTestCase.java | 2 + .../project/AbstractMavenProjectTestCase.java | 4 +- .../LegacyRepositorySystemTest.java | 1 + .../maven/internal/impl/DefaultSession.java | 23 +++---- .../internal/impl/DefaultSessionFactory.java | 4 +- .../maven/project/ProjectModelResolver.java | 60 ++++++++++--------- .../org/apache/maven/MavenTestHelper.java | 2 + .../apache/maven/internal/impl/TestApi.java | 1 + .../AbstractRepositoryTestCase.java | 2 + .../apache/maven/model/ModelBuilderTest.java | 1 + .../BootstrapCoreExtensionManager.java | 4 +- 14 files changed, 71 insertions(+), 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 index ca789501a7ad..3b9149289a44 100644 --- 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 toDependencies( return dependencies == null ? null : map(dependencies, d -> toDependency(d, managed)); } - protected List getRepositories( + static List getRepositories( List repositories, List 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 index 3d4107fc9448..e70ae40f4e1b 100644 --- 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 index 77d649585ae9..08c80cef4c0a 100644 --- 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 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 index 6ed8ba198e43..f4f9d134da03 100644 --- 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 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 index 987a6e3e2b34..56260a5edd4f 100644 --- 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 index ed477c9c3b44..586fcfbbcbb0 100644 --- 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 index 7a9fb8b95901..b848002ef3b2 100644 --- 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 toArtifactRepositories(List re @Nonnull @Override public Settings getSettings() { - return mavenSession.getSettings().getDelegate(); + return getMavenSession().getSettings().getDelegate(); } @Nonnull @Override public Map getUserProperties() { - return Collections.unmodifiableMap(new PropertiesAsMap(mavenSession.getUserProperties())); + return Collections.unmodifiableMap(new PropertiesAsMap(getMavenSession().getUserProperties())); } @Nonnull @Override public Map 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 getProjects() { - return getProjects(mavenSession.getProjects()); + return getProjects(getMavenSession().getProjects()); } @Nonnull @@ -161,14 +164,14 @@ public Map 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 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 index 9b6bf0692244..9e6cc6c1875f 100644 --- 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 index c0a091575260..0629a537cf57 100644 --- 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> parentCache; + private final Map> parentCache; public ProjectModelResolver( RepositorySystemSession session, @@ -189,38 +190,39 @@ public ModelSource resolveModel(final Parent parent, AtomicReference mod throws UnresolvableModelException { Result result; try { - ForkJoinTask future = parentCache.computeIfAbsent(parent.getId(), id -> new ForkJoinTask<>() { - Result result; - - @Override - public Result getRawResult() { - return result; - } + Future future = parentCache.computeIfAbsent(parent.getId(), id -> { + ForkJoinPool pool = new ForkJoinPool(MAX_CAP); + ForkJoinTask 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 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 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 index d3facca6a9e4..9cb2b347ef54 100644 --- 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 index fb4732b4071a..434d75846688 100644 --- 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/maven-core/src/test/java/org/apache/maven/internal/transformation/AbstractRepositoryTestCase.java b/maven-core/src/test/java/org/apache/maven/internal/transformation/AbstractRepositoryTestCase.java index 96821e98d1bf..ac48d1d91829 100644 --- a/maven-core/src/test/java/org/apache/maven/internal/transformation/AbstractRepositoryTestCase.java +++ b/maven-core/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 index 014226c1927f..f52cc73aab8f 100644 --- 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 results = projectBuilder.build( Collections.singletonList(new File("src/test/resources/projects/tree/pom.xml")), true, request); diff --git a/maven-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java b/maven-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java index 45b0962c25b2..ea647749cd78 100644 --- a/maven-embedder/src/main/java/org/apache/maven/cli/internal/BootstrapCoreExtensionManager.java +++ b/maven-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 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 repositories = RepositoryUtils.toRepos(request.getPluginArtifactRepositories()); Interpolator interpolator = createInterpolator(request);