From aeb02d2f7063ff6f5c818690c1251e1a1e1ced09 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Mon, 16 May 2022 11:26:49 +0200 Subject: [PATCH] [MNG-7476] Display a warning when an aggregator mojo is lock other mojos executions --- .../apache/maven/internal/MessageHelper.java | 91 +++++++++++++++++++ .../lifecycle/internal/MojoExecutor.java | 84 ++++++++++++++--- .../maven/internal/MessageHelperTest.java | 71 +++++++++++++++ 3 files changed, 232 insertions(+), 14 deletions(-) create mode 100644 maven-core/src/main/java/org/apache/maven/internal/MessageHelper.java create mode 100644 maven-core/src/test/java/org/apache/maven/internal/MessageHelperTest.java diff --git a/maven-core/src/main/java/org/apache/maven/internal/MessageHelper.java b/maven-core/src/main/java/org/apache/maven/internal/MessageHelper.java new file mode 100644 index 000000000000..f7049afbbd44 --- /dev/null +++ b/maven-core/src/main/java/org/apache/maven/internal/MessageHelper.java @@ -0,0 +1,91 @@ +package org.apache.maven.internal; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.ArrayList; +import java.util.List; + +/** + * Helper class to format warning messages to the console + */ +public class MessageHelper +{ + + private static final int DEFAULT_MAX_SIZE = 65; + private static final char BOX_CHAR = '*'; + + public static String separatorLine() + { + StringBuilder sb = new StringBuilder( DEFAULT_MAX_SIZE ); + repeat( sb, '*', DEFAULT_MAX_SIZE ); + return sb.toString(); + } + + public static List formatWarning( String... lines ) + { + int size = DEFAULT_MAX_SIZE; + int rem = size - 4; // 4 chars = 2 box_char + 2 spaces + List result = new ArrayList<>(); + StringBuilder sb = new StringBuilder( size ); + // first line + sb.setLength( 0 ); + repeat( sb, BOX_CHAR, size ); + result.add( sb.toString() ); + // lines + for ( String line : lines ) + { + sb.setLength( 0 ); + String[] words = line.split( " " ); + for ( String word : words ) + { + if ( sb.length() >= rem - word.length() - ( sb.length() > 0 ? 1 : 0 ) ) + { + repeat( sb, ' ', rem - sb.length() ); + result.add( BOX_CHAR + " " + sb + " " + BOX_CHAR ); + sb.setLength( 0 ); + } + if ( sb.length() > 0 ) + { + sb.append( ' ' ); + } + sb.append( word ); + } + + while ( sb.length() < rem ) + { + sb.append( ' ' ); + } + result.add( BOX_CHAR + " " + sb + " " + BOX_CHAR ); + } + // last line + sb.setLength( 0 ); + repeat( sb, BOX_CHAR, size ); + result.add( sb.toString() ); + return result; + } + + private static void repeat( StringBuilder sb, char c, int nb ) + { + for ( int i = 0; i < nb; i++ ) + { + sb.append( c ); + } + } +} 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 671ddbef6615..36f71829c1d6 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.MessageHelper; 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,9 @@ 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<>(); public MojoExecutor() { @@ -217,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 ); if ( session.getRequest().getDegreeOfConcurrency() > 1 ) { - boolean aggregator = mojoDescriptor.isAggregator(); - acquiredAggregatorLock = aggregator ? aggregatorLock.writeLock() : aggregatorLock.readLock(); + acquiredAggregatorLock = mojoDescriptor.isAggregator() + ? aggregatorLock.writeLock() : aggregatorLock.readLock(); + 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(); + } acquiredProjectLock = getProjectLock( session ); - acquiredAggregatorLock.lock(); - acquiredProjectLock.lock(); + if ( !acquiredProjectLock.tryLock() ) + { + Thread owner = acquiredProjectLock.getOwner(); + MojoDescriptor ownerMojo = owner != null ? mojos.get( owner ) : null; + String str = ownerMojo != null ? " The " + ownerMojo.getId() : "An "; + 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 { @@ -254,10 +284,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 ) { @@ -265,11 +295,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; @@ -279,6 +309,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 : MessageHelper.formatWarning( msg ) ) + { + LOGGER.warn( s ); + } + } + private void doExecute( MavenSession session, MojoExecution mojoExecution, ProjectIndex projectIndex, DependencyContext dependencyContext ) throws LifecycleExecutionException diff --git a/maven-core/src/test/java/org/apache/maven/internal/MessageHelperTest.java b/maven-core/src/test/java/org/apache/maven/internal/MessageHelperTest.java new file mode 100644 index 000000000000..791e4dc490c9 --- /dev/null +++ b/maven-core/src/test/java/org/apache/maven/internal/MessageHelperTest.java @@ -0,0 +1,71 @@ +package org.apache.maven.internal; + +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; + +public class MessageHelperTest +{ + + @Test + public void testBuilderCommon() + { + List msgs = new ArrayList<>(); + msgs.add( "*****************************************************************" ); + msgs.add( "* Your build is requesting parallel execution, but project *" ); + msgs.add( "* contains the following plugin(s) that have goals not marked *" ); + msgs.add( "* as @threadSafe to support parallel building. *" ); + msgs.add( "* While this /may/ work fine, please look for plugin updates *" ); + msgs.add( "* and/or request plugins be made thread-safe. *" ); + msgs.add( "* If reporting an issue, report it against the plugin in *" ); + msgs.add( "* question, not against maven-core *" ); + msgs.add( "*****************************************************************" ); + + assertEquals( msgs, MessageHelper.formatWarning( + "Your build is requesting parallel execution, but project contains the following " + + "plugin(s) that have goals not marked as @threadSafe to support parallel building.", + "While this /may/ work fine, please look for plugin updates and/or " + + "request plugins be made thread-safe.", + "If reporting an issue, report it against the plugin in question, not against maven-core" + ) ); + } + + @Test + public void testMojoExecutor() + { + List msgs = new ArrayList<>(); + msgs.add( "*****************************************************************" ); + msgs.add( "* An aggregator Mojo is already executing in parallel build, *" ); + msgs.add( "* but aggregator Mojos require exclusive access to reactor to *" ); + msgs.add( "* prevent race conditions. This mojo execution will be blocked *" ); + msgs.add( "* until the aggregator work is done. *" ); + msgs.add( "*****************************************************************" ); + + assertEquals( msgs, MessageHelper.formatWarning( + "An aggregator Mojo is already executing in parallel build, but aggregator " + + "Mojos require exclusive access to reactor to prevent race conditions. This " + + "mojo execution will be blocked until the aggregator work is done." ) ); + } +}