Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.8.x] [MNG-7476] Display a warning when an aggregator mojo locks other mojos executions #736

Merged
merged 2 commits into from
May 30, 2022
Merged

[3.8.x] [MNG-7476] Display a warning when an aggregator mojo locks other mojos executions #736

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67426e9
[MNG-7487] Fix deadlock during forked lifecycle executions
gnodet May 12, 2022
393a5fa
[MNG-7476] Display a warning when an aggregator mojo is locking other…
gnodet May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 86 additions & 24 deletions maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;

Expand All @@ -71,6 +73,8 @@
public class MojoExecutor
{

private static final Logger LOGGER = LoggerFactory.getLogger( MojoExecutor.class );

@Requirement
private BuildPluginManager pluginManager;

Expand All @@ -83,7 +87,9 @@ public class MojoExecutor
@Requirement
private ExecutionEventCatapult eventCatapult;

private final ReadWriteLock aggregatorLock = new ReentrantReadWriteLock();
private final OwnerReentrantReadWriteLock aggregatorLock = new OwnerReentrantReadWriteLock();

private final Map<Thread, MojoDescriptor> mojos = new ConcurrentHashMap<>();

public MojoExecutor()
{
Expand Down Expand Up @@ -206,10 +212,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 );
}

/**
Expand All @@ -220,20 +223,44 @@ 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;
final OwnerReentrantLock acquiredProjectLock;

ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor, ReadWriteLock aggregatorLock )
ProjectLock( MavenSession session, MojoDescriptor mojoDescriptor )
{
mojos.put( Thread.currentThread(), mojoDescriptor );
michael-o marked this conversation as resolved.
Show resolved Hide resolved
if ( session.getRequest().getDegreeOfConcurrency() > 1 )
{
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
{
Expand All @@ -254,25 +281,26 @@ public void close()
{
acquiredAggregatorLock.unlock();
}
mojos.remove( Thread.currentThread() );
}

@SuppressWarnings( { "unchecked", "rawtypes" } )
private Lock getProjectLock( MavenSession session )
private OwnerReentrantLock getProjectLock( MavenSession session )
{
SessionData data = session.getRepositorySession().getData();
ConcurrentMap<MavenProject, Lock> locks = ( ConcurrentMap ) data.get( ProjectLock.class );
ConcurrentMap<MavenProject, OwnerReentrantLock> 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() );
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;
Expand All @@ -282,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 );
}
gnodet marked this conversation as resolved.
Show resolved Hide resolved
}

private void doExecute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex,
DependencyContext dependencyContext )
throws LifecycleExecutionException
Expand All @@ -292,8 +346,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
Expand All @@ -314,13 +383,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,
Expand Down