From a6e462b53a4b6c87ef43f6cfe7fbde0b0939e3d6 Mon Sep 17 00:00:00 2001 From: Falko Modler Date: Sun, 12 Sep 2021 22:59:48 +0200 Subject: [PATCH] [MNG-7251] Fix threadLocalArtifactsHolder leaking into cloned project This closes #535 --- .../apache/maven/project/MavenProject.java | 33 ++++++-------- .../maven/project/MavenProjectTest.java | 43 +++++++++++++++++++ 2 files changed, 57 insertions(+), 19 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 0fce71a68e..83b86c36f1 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 @@ -147,8 +147,7 @@ public class MavenProject private Artifact artifact; - private final ThreadLocal threadLocalArtifactsHolder = - ThreadLocal.withInitial( ArtifactsHolder::new ); + private ThreadLocal threadLocalArtifactsHolder = ThreadLocal.withInitial( ArtifactsHolder::new ); private Model originalModel; @@ -1177,7 +1176,8 @@ public class MavenProject { throw new UnsupportedOperationException( e ); } - + // clone must have its own TL, otherwise the artifacts are intermingled! + clone.threadLocalArtifactsHolder = ThreadLocal.withInitial( ArtifactsHolder::new ); clone.deepCopy( this ); return clone; @@ -1220,6 +1220,7 @@ public class MavenProject // 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! @@ -1228,11 +1229,6 @@ public class MavenProject setDependencyArtifacts( Collections.unmodifiableSet( project.getDependencyArtifacts() ) ); } - if ( project.getArtifacts() != null ) - { - setArtifacts( Collections.unmodifiableSet( project.getArtifacts() ) ); - } - if ( project.getParentFile() != null ) { parentFile = new File( project.getParentFile().getAbsolutePath() ); @@ -1475,13 +1471,7 @@ public class MavenProject // ---------------------------------------------------------------------------------------------------------------- // - // - // 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 // // ---------------------------------------------------------------------------------------------------------------- @@ -1502,7 +1492,6 @@ public class MavenProject if ( moduleFile != null ) { File moduleDir = moduleFile.getCanonicalFile().getParentFile(); - module = moduleDir.getName(); } @@ -1823,7 +1812,6 @@ public class MavenProject public void setReportArtifacts( Set reportArtifacts ) { this.reportArtifacts = reportArtifacts; - reportArtifactMap = null; } @@ -1847,7 +1835,6 @@ public class MavenProject public void setExtensionArtifacts( Set extensionArtifacts ) { this.extensionArtifacts = extensionArtifacts; - extensionArtifactMap = null; } @@ -1881,7 +1868,6 @@ public class MavenProject 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 @@ -1991,5 +1977,14 @@ public class MavenProject 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 d2cba20a41..aff0dde386 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,14 +21,19 @@ package org.apache.maven.project; 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.junit.jupiter.api.Test; +import org.mockito.Mockito; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; @@ -204,6 +209,44 @@ public class MavenProjectTest } @Test + public void testCloneWithArtifacts() + throws InterruptedException + { + Artifact initialArtifact = Mockito.mock( Artifact.class, "initialArtifact" ); + MavenProject originalProject = new MavenProject(); + originalProject.setArtifacts( Collections.singleton( initialArtifact ) ); + assertEquals( Collections.singleton( initialArtifact ), originalProject.getArtifacts(), + "Sanity check: originalProject returns artifact that has just been set" ); + + final MavenProject clonedProject = originalProject.clone(); + + assertEquals( Collections.singleton( initialArtifact ), clonedProject.getArtifacts(), + "Cloned project returns the artifact that was set for the original project" ); + + Artifact anotherArtifact = Mockito.mock( Artifact.class, "anotherArtifact" ); + clonedProject.setArtifacts( Collections.singleton( anotherArtifact ) ); + assertEquals( Collections.singleton( anotherArtifact ), clonedProject.getArtifacts(), + "Sanity check: clonedProject returns artifact that has just been set" ); + + assertEquals( Collections.singleton( initialArtifact ), originalProject.getArtifacts(), + "Original project returns the artifact that was set initially (not the one for clonedProject)" ); + + final AtomicReference> artifactsFromThread = new AtomicReference<>(); + Thread thread = new Thread( new Runnable() + { + @Override + public void run() + { + artifactsFromThread.set( clonedProject.getArtifacts() ); + } + } ); + thread.start(); + thread.join(); + + assertEquals( Collections.emptySet(), artifactsFromThread.get(), + "Another thread does not see the same artifacts" ); + } + public void testUndefinedOutputDirectory() throws Exception {