From 4e5b3d55545e5f03f05ac7b0cd1b56689df36201 Mon Sep 17 00:00:00 2001 From: Falko Modler Date: Sun, 22 Aug 2021 22:52:30 +0200 Subject: [PATCH] [MNG-7251] Fix threadLocalArtifactsHolder leaking into cloned project This closes #527 --- .../apache/maven/project/MavenProject.java | 49 ++++++++++--------- .../maven/project/MavenProjectTest.java | 43 ++++++++++++++++ 2 files changed, 68 insertions(+), 24 deletions(-) diff --git a/maven-core/src/main/java/org/apache/maven/project/MavenProject.java b/maven-core/src/main/java/org/apache/maven/project/MavenProject.java index 157b7a0f0a5..94d67881453 100644 --- a/maven-core/src/main/java/org/apache/maven/project/MavenProject.java +++ b/maven-core/src/main/java/org/apache/maven/project/MavenProject.java @@ -143,13 +143,7 @@ public class MavenProject private Artifact artifact; - private final ThreadLocal threadLocalArtifactsHolder = new ThreadLocal() - { - protected ArtifactsHolder initialValue() - { - return new ArtifactsHolder(); - } - }; + private ThreadLocal threadLocalArtifactsHolder = newThreadLocalArtifactsHolder(); private Model originalModel; @@ -188,6 +182,17 @@ public MavenProject() setModel( model ); } + private static ThreadLocal newThreadLocalArtifactsHolder() + { + return new ThreadLocal() + { + protected ArtifactsHolder initialValue() + { + return new ArtifactsHolder(); + } + }; + } + public MavenProject( Model model ) { setModel( model ); @@ -1181,7 +1186,8 @@ public MavenProject clone() { throw new UnsupportedOperationException( e ); } - + // clone must have it's own TL, otherwise the artifacts are intermingled! + clone.threadLocalArtifactsHolder = newThreadLocalArtifactsHolder(); clone.deepCopy( this ); return clone; @@ -1224,6 +1230,7 @@ private void deepCopy( MavenProject project ) // copy fields file = project.file; basedir = project.basedir; + threadLocalArtifactsHolder.set( project.threadLocalArtifactsHolder.get().copy() ); // don't need a deep copy, they don't get modified or added/removed to/from - but make them unmodifiable to be // sure! @@ -1232,11 +1239,6 @@ private void deepCopy( MavenProject project ) setDependencyArtifacts( Collections.unmodifiableSet( project.getDependencyArtifacts() ) ); } - if ( project.getArtifacts() != null ) - { - setArtifacts( Collections.unmodifiableSet( project.getArtifacts() ) ); - } - if ( project.getParentFile() != null ) { parentFile = new File( project.getParentFile().getAbsolutePath() ); @@ -1479,13 +1481,7 @@ public void addLifecyclePhase( String lifecyclePhase ) // ---------------------------------------------------------------------------------------------------------------- // - // - // D E P R E C A T E D - // - // - // ---------------------------------------------------------------------------------------------------------------- - // - // Everything below will be removed for Maven 4.0.0 + // D E P R E C A T E D - Everything below will be removed for Maven 4.0.0 // // ---------------------------------------------------------------------------------------------------------------- @@ -1506,7 +1502,6 @@ public String getModulePathAdjustment( MavenProject moduleProject ) if ( moduleFile != null ) { File moduleDir = moduleFile.getCanonicalFile().getParentFile(); - module = moduleDir.getName(); } @@ -1827,7 +1822,6 @@ public Reporting getReporting() public void setReportArtifacts( Set reportArtifacts ) { this.reportArtifacts = reportArtifacts; - reportArtifactMap = null; } @@ -1851,7 +1845,6 @@ public Map getReportArtifactMap() public void setExtensionArtifacts( Set extensionArtifacts ) { this.extensionArtifacts = extensionArtifacts; - extensionArtifactMap = null; } @@ -1885,7 +1878,6 @@ public List getReportPlugins() public Xpp3Dom getReportConfiguration( String pluginGroupId, String pluginArtifactId, String reportSetId ) { Xpp3Dom dom = null; - // ---------------------------------------------------------------------- // I would like to be able to lookup the Mojo object using a key but // we have a limitation in modello that will be remedied shortly. So @@ -1995,5 +1987,14 @@ private static class ArtifactsHolder private ArtifactFilter artifactFilter; private Set artifacts; private Map artifactMap; + + ArtifactsHolder copy() + { + ArtifactsHolder copy = new ArtifactsHolder(); + copy.artifactFilter = artifactFilter; + copy.artifacts = artifacts != null ? new LinkedHashSet<>( artifacts ) : null; + copy.artifactMap = artifactMap != null ? new LinkedHashMap<>( artifactMap ) : null; + return copy; + } } } diff --git a/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java b/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java index 6b4258b3fde..2344d8f7734 100644 --- a/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java +++ b/maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java @@ -21,13 +21,18 @@ import java.io.File; import java.io.IOException; +import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.maven.artifact.Artifact; import org.apache.maven.model.DependencyManagement; import org.apache.maven.model.Model; import org.apache.maven.model.Parent; import org.apache.maven.model.Profile; +import org.mockito.Mockito; public class MavenProjectTest extends AbstractMavenProjectTestCase @@ -188,6 +193,44 @@ public void testCloneWithBaseDir() assertEquals( "Base directory is preserved across clone", projectToClone.getBasedir(), clonedProject.getBasedir() ); } + public void testCloneWithArtifacts() + throws InterruptedException + { + Artifact initialArtifact = Mockito.mock( Artifact.class, "initialArtifact" ); + MavenProject originalProject = new MavenProject(); + originalProject.setArtifacts( Collections.singleton( initialArtifact ) ); + assertEquals( "Sanity check: originalProject returns artifact that has just been set", + Collections.singleton( initialArtifact ), originalProject.getArtifacts() ); + + final MavenProject clonedProject = originalProject.clone(); + + assertEquals( "Cloned project returns the artifact that was set for the original project", + Collections.singleton( initialArtifact ), clonedProject.getArtifacts() ); + + Artifact anotherArtifact = Mockito.mock( Artifact.class, "anotherArtifact" ); + clonedProject.setArtifacts( Collections.singleton( anotherArtifact ) ); + assertEquals( "Sanity check: clonedProject returns artifact that has just been set", + Collections.singleton( anotherArtifact ), clonedProject.getArtifacts() ); + + assertEquals( "Original project returns the artifact that was set initially (not the one for clonedProject)", + Collections.singleton( initialArtifact ), originalProject.getArtifacts() ); + + final AtomicReference> artifactsFromThread = new AtomicReference<>(); + Thread thread = new Thread( new Runnable() + { + @Override + public void run() + { + artifactsFromThread.set( clonedProject.getArtifacts() ); + } + } ); + thread.start(); + thread.join(); + + assertEquals( "Another thread does not see the same artifacts", + Collections.emptySet(), artifactsFromThread.get() ); + } + public void testUndefinedOutputDirectory() throws Exception {