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

[MNG-7220] [3.8.x] Fix threadLocalArtifactsHolder leaking into cloned project #527

Closed
wants to merge 1 commit into from
Closed

[MNG-7220] [3.8.x] Fix threadLocalArtifactsHolder leaking into cloned project #527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
49 changes: 25 additions & 24 deletions maven-core/src/main/java/org/apache/maven/project/MavenProject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,7 @@ public class MavenProject

private Artifact artifact;

private final ThreadLocal<ArtifactsHolder> threadLocalArtifactsHolder = new ThreadLocal()
{
protected ArtifactsHolder initialValue()
{
return new ArtifactsHolder();
}
};
private ThreadLocal<ArtifactsHolder> threadLocalArtifactsHolder = newThreadLocalArtifactsHolder();

private Model originalModel;

Expand Down Expand Up @@ -188,6 +182,17 @@ public MavenProject()
setModel( model );
}

private static ThreadLocal<ArtifactsHolder> newThreadLocalArtifactsHolder()
{
return new ThreadLocal<ArtifactsHolder>()
{
protected ArtifactsHolder initialValue()
{
return new ArtifactsHolder();
}
};
}

public MavenProject( Model model )
{
setModel( model );
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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!
Expand All @@ -1232,11 +1239,6 @@ private void deepCopy( MavenProject project )
setDependencyArtifacts( Collections.unmodifiableSet( project.getDependencyArtifacts() ) );
}

if ( project.getArtifacts() != null )
michael-o marked this conversation as resolved.
Show resolved Hide resolved
{
setArtifacts( Collections.unmodifiableSet( project.getArtifacts() ) );
michael-o marked this conversation as resolved.
Show resolved Hide resolved
}

if ( project.getParentFile() != null )
{
parentFile = new File( project.getParentFile().getAbsolutePath() );
Expand Down Expand Up @@ -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
//
// ----------------------------------------------------------------------------------------------------------------

Expand All @@ -1506,7 +1502,6 @@ public String getModulePathAdjustment( MavenProject moduleProject )
if ( moduleFile != null )
{
File moduleDir = moduleFile.getCanonicalFile().getParentFile();

module = moduleDir.getName();
}

Expand Down Expand Up @@ -1827,7 +1822,6 @@ public Reporting getReporting()
public void setReportArtifacts( Set<Artifact> reportArtifacts )
{
this.reportArtifacts = reportArtifacts;

reportArtifactMap = null;
}

Expand All @@ -1851,7 +1845,6 @@ public Map<String, Artifact> getReportArtifactMap()
public void setExtensionArtifacts( Set<Artifact> extensionArtifacts )
{
this.extensionArtifacts = extensionArtifacts;

extensionArtifactMap = null;
}

Expand Down Expand Up @@ -1885,7 +1878,6 @@ public List<ReportPlugin> 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
Expand Down Expand Up @@ -1995,5 +1987,14 @@ private static class ArtifactsHolder
private ArtifactFilter artifactFilter;
private Set<Artifact> artifacts;
private Map<String, Artifact> 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;
}
}
}
43 changes: 43 additions & 0 deletions maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<Set<Artifact>> 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
{
Expand Down