From c963ae5204b7196762fcdf5ee554173602908c66 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 3 Dec 2021 09:44:51 +0100 Subject: [PATCH 1/2] [MNG-7156][MNG-7285] Add locking in MojoExecutor --- .../lifecycle/internal/MojoExecutor.java | 91 +++++++++++++++++++ .../maven/project/ProjectBuilderTest.java | 12 +++ 2 files changed, 103 insertions(+) diff --git a/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java b/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java index b78f54dc42f3..cf97c8cab954 100644 --- a/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java +++ b/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java @@ -39,6 +39,7 @@ import org.codehaus.plexus.component.annotations.Component; import org.codehaus.plexus.component.annotations.Requirement; import org.codehaus.plexus.util.StringUtils; +import org.eclipse.aether.SessionData; import java.util.ArrayList; import java.util.Arrays; @@ -48,6 +49,12 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; /** *

@@ -76,6 +83,8 @@ public class MojoExecutor @Requirement private ExecutionEventCatapult eventCatapult; + private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock(); + public MojoExecutor() { } @@ -197,6 +206,88 @@ private void execute( MavenSession session, MojoExecution mojoExecution, Project } } + try ( ProjectLock lock = new ProjectLock( session, mojoDescriptor, aggregatorLock ) ) + { + doExecute( session, mojoExecution, projectIndex, dependencyContext ); + } + } + + /** + * Aggregating mojo executions (possibly) modify all MavenProjects, including those that are currently in use + * by concurrently running mojo executions. To prevent race conditions, an aggregating execution will block + * all other executions until finished. + * We also lock on a given project to forbid a forked lifecycle to be executed concurrently with the project. + * TODO: ideally, the builder should take care of the ordering in a smarter way + * TODO: and concurrency issues fixed with MNG-7157 + */ + private static class ProjectLock implements AutoCloseable + { + final Lock acquiredAggregatorLock; + final Lock acquiredProjectLock; + + ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor, ReadWriteLock aggregatorLock ) + { + if ( session.getRequest().getDegreeOfConcurrency() > 1 ) + { + boolean aggregator = mojoDescriptor.isAggregator(); + acquiredAggregatorLock = aggregator ? aggregatorLock.writeLock() : aggregatorLock.readLock(); + acquiredProjectLock = getProjectLock( session ); + acquiredAggregatorLock.lock(); + acquiredProjectLock.lock(); + } + else + { + acquiredAggregatorLock = null; + acquiredProjectLock = null; + } + } + + @Override + public void close() + { + // release the lock in the reverse order of the acquisition + if ( acquiredProjectLock != null ) + { + acquiredProjectLock.unlock(); + } + if ( acquiredAggregatorLock != null ) + { + acquiredAggregatorLock.unlock(); + } + } + + @SuppressWarnings( { "unchecked", "rawtypes" } ) + private Lock getProjectLock( MavenSession session ) + { + SessionData data = session.getRepositorySession().getData(); + ConcurrentMap locks = ( ConcurrentMap ) data.get( ProjectLock.class ); + // initialize the value if not already done (in case of a concurrent access) to the method + if ( locks == null ) + { + // the call to data.set(k, null, v) is effectively a call to data.putIfAbsent(k, v) + data.set( ProjectLock.class, null, new ConcurrentHashMap<>() ); + locks = ( ConcurrentMap ) data.get( ProjectLock.class ); + } + Lock acquiredProjectLock = locks.get( session.getCurrentProject() ); + if ( acquiredProjectLock == null ) + { + acquiredProjectLock = new ReentrantLock(); + Lock prev = locks.putIfAbsent( session.getCurrentProject(), acquiredProjectLock ); + if ( prev != null ) + { + acquiredProjectLock = prev; + } + } + return acquiredProjectLock; + } + } + + private void doExecute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex, + DependencyContext dependencyContext ) + throws LifecycleExecutionException + { + MojoDescriptor mojoDescriptor = mojoExecution.getMojoDescriptor(); + List forkedProjects = executeForkedExecutions( mojoExecution, session, projectIndex ); ensureDependenciesAreResolved( mojoDescriptor, session, dependencyContext ); diff --git a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java index 6adb10e8f4e2..1780027b04e3 100644 --- a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java +++ b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java @@ -119,6 +119,18 @@ public void testResolveDependencies() assertEquals( 1, results.size() ); MavenProject mavenProject = results.get( 0 ).getProject(); assertEquals( 1, mavenProject.getArtifacts().size() ); + + final MavenProject project = mavenProject; + final int[] getArtifactsResultInAnotherThead = { 0 }; + Thread t = new Thread(new Runnable() { + @Override + public void run() { + getArtifactsResultInAnotherThead[0] = project.getArtifacts().size(); + } + }); + t.start(); + t.join(); + assertEquals( project.getArtifacts().size(), getArtifactsResultInAnotherThead[0] ); } public void testDontResolveDependencies() From a2d437a1905cc8850c6b78c0e28fb5764ec36100 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 15 Dec 2021 07:24:19 +0100 Subject: [PATCH 2/2] Use an AtomicInteger instead of an int[] --- .../java/org/apache/maven/project/ProjectBuilderTest.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java index 1780027b04e3..40ba60dadfeb 100644 --- a/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java +++ b/maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java @@ -32,6 +32,7 @@ import java.util.Collections; import java.util.List; import java.util.Properties; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.maven.AbstractCoreMavenComponentTestCase; import org.apache.maven.artifact.InvalidArtifactRTException; @@ -121,16 +122,16 @@ public void testResolveDependencies() assertEquals( 1, mavenProject.getArtifacts().size() ); final MavenProject project = mavenProject; - final int[] getArtifactsResultInAnotherThead = { 0 }; + final AtomicInteger artifactsResultInAnotherThead = new AtomicInteger(); Thread t = new Thread(new Runnable() { @Override public void run() { - getArtifactsResultInAnotherThead[0] = project.getArtifacts().size(); + artifactsResultInAnotherThead.set(project.getArtifacts().size()); } }); t.start(); t.join(); - assertEquals( project.getArtifacts().size(), getArtifactsResultInAnotherThead[0] ); + assertEquals( project.getArtifacts().size(), artifactsResultInAnotherThead.get() ); } public void testDontResolveDependencies()