From 3330b7033f0ee8a04e431734519dd070c72879aa Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 2 Dec 2021 21:23:34 +0100 Subject: [PATCH 1/7] [MNG-7156][MNG-7285] Add locking in MojoExecutor --- .../lifecycle/internal/MojoExecutor.java | 82 +++++++++++++++++++ 1 file changed, 82 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 946bffb0a104..632723563908 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 @@ -27,6 +27,11 @@ import java.util.Map; import java.util.Set; import java.util.TreeSet; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import javax.inject.Inject; import javax.inject.Named; @@ -50,6 +55,7 @@ import org.apache.maven.plugin.descriptor.MojoDescriptor; import org.apache.maven.project.MavenProject; import org.codehaus.plexus.util.StringUtils; +import org.eclipse.aether.SessionData; /** *

@@ -72,6 +78,8 @@ public class MojoExecutor private final LifecycleDependencyResolver lifeCycleDependencyResolver; private final ExecutionEventCatapult eventCatapult; + private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock( true ); + @Inject public MojoExecutor( BuildPluginManager pluginManager, @@ -202,6 +210,80 @@ 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 ) + { + final Lock acquiredProjectLock; + SessionData data = session.getRepositorySession().getData(); + Map locks = ( Map ) 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 = ( Map ) data.get( ProjectLock.class ); + } + acquiredProjectLock = locks.computeIfAbsent( session.getCurrentProject(), p -> new ReentrantLock() ); + 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 ); From 87c0a8dea6a1714d268223cf1237e828585906a5 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 2 Dec 2021 22:48:05 +0100 Subject: [PATCH 2/7] [MNG-7285] - Test showcasing regression #570 --- .../org/apache/maven/project/ProjectBuilderTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 fbe1a4e2b799..cf863318b30e 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 @@ -127,6 +127,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] ); } @Test From 917f9b5e65cdde86a330e92a53b5ef5ea1b786e0 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 2 Dec 2021 22:52:34 +0100 Subject: [PATCH 3/7] No real need for a fair lock --- .../java/org/apache/maven/lifecycle/internal/MojoExecutor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 632723563908..82ca944ab1c0 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 @@ -78,7 +78,7 @@ public class MojoExecutor private final LifecycleDependencyResolver lifeCycleDependencyResolver; private final ExecutionEventCatapult eventCatapult; - private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock( true ); + private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock(); @Inject public MojoExecutor( From 118bb098a82de133c093f2dd366e3b1482668449 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 3 Dec 2021 08:24:07 +0100 Subject: [PATCH 4/7] Add a comment to improve the code when resolver 1.7.3 is released --- .../java/org/apache/maven/lifecycle/internal/MojoExecutor.java | 3 +++ 1 file changed, 3 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 82ca944ab1c0..eabb9c3db40b 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 @@ -265,6 +265,9 @@ private Lock getProjectLock( MavenSession session ) { final Lock acquiredProjectLock; SessionData data = session.getRepositorySession().getData(); + // TODO: when resolver 1.7.3 is released, the code below should be changed to + // TODO: Map locks = ( Map ) ((Map) data).computeIfAbsent( + // TODO: ProjectLock.class, l -> new ConcurrentHashMap<>() ); Map locks = ( Map ) data.get( ProjectLock.class ); // initialize the value if not already done (in case of a concurrent access) to the method if ( locks == null ) From c644963eff0c2cc4d00932d21663d114ba53a460 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 3 Dec 2021 09:57:26 +0100 Subject: [PATCH 5/7] Small cleanup --- .../org/apache/maven/lifecycle/internal/MojoExecutor.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 eabb9c3db40b..a06cd9f40efd 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 @@ -263,7 +263,6 @@ public void close() @SuppressWarnings( { "unchecked", "rawtypes" } ) private Lock getProjectLock( MavenSession session ) { - final Lock acquiredProjectLock; SessionData data = session.getRepositorySession().getData(); // TODO: when resolver 1.7.3 is released, the code below should be changed to // TODO: Map locks = ( Map ) ((Map) data).computeIfAbsent( @@ -276,8 +275,7 @@ private Lock getProjectLock( MavenSession session ) data.set( ProjectLock.class, null, new ConcurrentHashMap<>() ); locks = ( Map ) data.get( ProjectLock.class ); } - acquiredProjectLock = locks.computeIfAbsent( session.getCurrentProject(), p -> new ReentrantLock() ); - return acquiredProjectLock; + return locks.computeIfAbsent( session.getCurrentProject(), p -> new ReentrantLock() ); } } From b6cc51cee38f8d4949113d2274acda6b08154562 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 15 Dec 2021 07:24:19 +0100 Subject: [PATCH 6/7] 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 cf863318b30e..6adf48ce145e 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 @@ -26,6 +26,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.execution.MavenSession; @@ -129,16 +130,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() ); } @Test From d60036872e8ae250b16c200f081f2b830f12bcfd Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Fri, 17 Dec 2021 08:49:21 +0100 Subject: [PATCH 7/7] Fix typo --- .../java/org/apache/maven/project/ProjectBuilderTest.java | 6 +++--- 1 file changed, 3 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 6adf48ce145e..53b0af88b09a 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 @@ -130,16 +130,16 @@ public void testResolveDependencies() assertEquals( 1, mavenProject.getArtifacts().size() ); final MavenProject project = mavenProject; - final AtomicInteger artifactsResultInAnotherThead = new AtomicInteger(); + final AtomicInteger artifactsResultInAnotherThread = new AtomicInteger(); Thread t = new Thread(new Runnable() { @Override public void run() { - artifactsResultInAnotherThead.set(project.getArtifacts().size()); + artifactsResultInAnotherThread.set(project.getArtifacts().size()); } }); t.start(); t.join(); - assertEquals( project.getArtifacts().size(), artifactsResultInAnotherThead.get() ); + assertEquals( project.getArtifacts().size(), artifactsResultInAnotherThread.get() ); } @Test