From 67426e9a99dc4feff9268721c6584c322c6326b1 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 12 May 2022 10:04:51 +0200 Subject: [PATCH 1/2] [MNG-7487] Fix deadlock during forked lifecycle executions --- .../lifecycle/internal/MojoExecutor.java | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 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 cf97c8cab95..beca2eff014 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 @@ -85,6 +85,8 @@ public class MojoExecutor private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock(); + private final Map mojos = new ConcurrentHashMap<>(); + public MojoExecutor() { } @@ -206,10 +208,7 @@ private void execute( MavenSession session, MojoExecution mojoExecution, Project } } - try ( ProjectLock lock = new ProjectLock( session, mojoDescriptor, aggregatorLock ) ) - { - doExecute( session, mojoExecution, projectIndex, dependencyContext ); - } + doExecute( session, mojoExecution, projectIndex, dependencyContext ); } /** @@ -220,13 +219,14 @@ private void execute( MavenSession session, MojoExecution mojoExecution, 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 + private class ProjectLock implements AutoCloseable { final Lock acquiredAggregatorLock; final Lock acquiredProjectLock; - ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor, ReadWriteLock aggregatorLock ) + ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor ) { + mojos.put( Thread.currentThread(), mojoDescriptor ); if ( session.getRequest().getDegreeOfConcurrency() > 1 ) { boolean aggregator = mojoDescriptor.isAggregator(); @@ -254,6 +254,7 @@ public void close() { acquiredAggregatorLock.unlock(); } + mojos.remove( Thread.currentThread() ); } @SuppressWarnings( { "unchecked", "rawtypes" } ) @@ -292,8 +293,23 @@ private void doExecute( MavenSession session, MojoExecution mojoExecution, Proje ensureDependenciesAreResolved( mojoDescriptor, session, dependencyContext ); - eventCatapult.fire( ExecutionEvent.Type.MojoStarted, session, mojoExecution ); + try ( ProjectLock lock = new ProjectLock( session, mojoDescriptor ) ) + { + doExecute2( session, mojoExecution ); + } + finally + { + for ( MavenProject forkedProject : forkedProjects ) + { + forkedProject.setExecutionProject( null ); + } + } + } + private void doExecute2( MavenSession session, MojoExecution mojoExecution ) + throws LifecycleExecutionException + { + eventCatapult.fire( ExecutionEvent.Type.MojoStarted, session, mojoExecution ); try { try @@ -314,13 +330,6 @@ private void doExecute( MavenSession session, MojoExecution mojoExecution, Proje throw e; } - finally - { - for ( MavenProject forkedProject : forkedProjects ) - { - forkedProject.setExecutionProject( null ); - } - } } public void ensureDependenciesAreResolved( MojoDescriptor mojoDescriptor, MavenSession session, From 393a5fa856c394ea0104f45c75db0bdc2b5997e3 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Mon, 16 May 2022 11:26:49 +0200 Subject: [PATCH 2/2] [MNG-7476] Display a warning when an aggregator mojo is locking other mojos executions --- .../lifecycle/internal/MojoExecutor.java | 73 ++++++++++++++++--- 1 file changed, 63 insertions(+), 10 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 beca2eff014..3ef77844b7a 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 @@ -24,6 +24,7 @@ import org.apache.maven.artifact.resolver.filter.CumulativeScopeArtifactFilter; import org.apache.maven.execution.ExecutionEvent; import org.apache.maven.execution.MavenSession; +import org.apache.maven.internal.MultilineMessageHelper; import org.apache.maven.lifecycle.LifecycleExecutionException; import org.apache.maven.lifecycle.MissingProjectException; import org.apache.maven.plugin.BuildPluginManager; @@ -40,6 +41,8 @@ import org.codehaus.plexus.component.annotations.Requirement; import org.codehaus.plexus.util.StringUtils; import org.eclipse.aether.SessionData; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.Arrays; @@ -52,7 +55,6 @@ 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; @@ -71,6 +73,8 @@ public class MojoExecutor { + private static final Logger LOGGER = LoggerFactory.getLogger( MojoExecutor.class ); + @Requirement private BuildPluginManager pluginManager; @@ -83,7 +87,7 @@ public class MojoExecutor @Requirement private ExecutionEventCatapult eventCatapult; - private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock(); + private final OwnerReentrantReadWriteLock aggregatorLock = new OwnerReentrantReadWriteLock(); private final Map mojos = new ConcurrentHashMap<>(); @@ -222,7 +226,7 @@ private void execute( MavenSession session, MojoExecution mojoExecution, Project private class ProjectLock implements AutoCloseable { final Lock acquiredAggregatorLock; - final Lock acquiredProjectLock; + final OwnerReentrantLock acquiredProjectLock; ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor ) { @@ -232,8 +236,31 @@ private class ProjectLock implements AutoCloseable boolean aggregator = mojoDescriptor.isAggregator(); acquiredAggregatorLock = aggregator ? aggregatorLock.writeLock() : aggregatorLock.readLock(); acquiredProjectLock = getProjectLock( session ); - acquiredAggregatorLock.lock(); - acquiredProjectLock.lock(); + if ( !acquiredAggregatorLock.tryLock() ) + { + Thread owner = aggregatorLock.getOwner(); + MojoDescriptor ownerMojo = owner != null ? mojos.get( owner ) : null; + String str = ownerMojo != null ? " The " + ownerMojo.getId() : "An"; + String msg = str + " aggregator mojo is already being executed " + + "in this parallel build, those kind of mojos require exclusive access to " + + "reactor to prevent race conditions. This mojo execution will be blocked " + + "until the aggregator mojo is done."; + warn( msg ); + acquiredAggregatorLock.lock(); + } + if ( !acquiredProjectLock.tryLock() ) + { + Thread owner = acquiredProjectLock.getOwner(); + MojoDescriptor ownerMojo = owner != null ? mojos.get( owner ) : null; + String str = ownerMojo != null ? " The " + ownerMojo.getId() : "A"; + String msg = str + " mojo is already being executed " + + "on the project " + session.getCurrentProject().getGroupId() + + ":" + session.getCurrentProject().getArtifactId() + ". " + + "This mojo execution will be blocked " + + "until the mojo is done."; + warn( msg ); + acquiredProjectLock.lock(); + } } else { @@ -258,10 +285,10 @@ public void close() } @SuppressWarnings( { "unchecked", "rawtypes" } ) - private Lock getProjectLock( MavenSession session ) + private OwnerReentrantLock getProjectLock( MavenSession session ) { SessionData data = session.getRepositorySession().getData(); - ConcurrentMap locks = ( ConcurrentMap ) data.get( ProjectLock.class ); + 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 ) { @@ -269,11 +296,11 @@ private Lock getProjectLock( MavenSession session ) data.set( ProjectLock.class, null, new ConcurrentHashMap<>() ); locks = ( ConcurrentMap ) data.get( ProjectLock.class ); } - Lock acquiredProjectLock = locks.get( session.getCurrentProject() ); + OwnerReentrantLock acquiredProjectLock = locks.get( session.getCurrentProject() ); if ( acquiredProjectLock == null ) { - acquiredProjectLock = new ReentrantLock(); - Lock prev = locks.putIfAbsent( session.getCurrentProject(), acquiredProjectLock ); + acquiredProjectLock = new OwnerReentrantLock(); + OwnerReentrantLock prev = locks.putIfAbsent( session.getCurrentProject(), acquiredProjectLock ); if ( prev != null ) { acquiredProjectLock = prev; @@ -283,6 +310,32 @@ private Lock getProjectLock( MavenSession session ) } } + static class OwnerReentrantLock extends ReentrantLock + { + @Override + public Thread getOwner() + { + return super.getOwner(); + } + } + + static class OwnerReentrantReadWriteLock extends ReentrantReadWriteLock + { + @Override + public Thread getOwner() + { + return super.getOwner(); + } + } + + private static void warn( String msg ) + { + for ( String s : MultilineMessageHelper.format( msg ) ) + { + LOGGER.warn( s ); + } + } + private void doExecute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex, DependencyContext dependencyContext ) throws LifecycleExecutionException