Skip to content

Commit

Permalink
[MNG-7312] Revert ThreadLocal approach from MNG-6843 and MNG-7251
Browse files Browse the repository at this point in the history 
Revert "[MNG-7251] Fix threadLocalArtifactsHolder leaking into cloned project"

This reverts commit 4e5b3d5.

Revert "[MNG-6843] Parallel build fails due to missing JAR artifacts in compilePath"

This reverts commit 76d5f0d.

===

This closes #595
  • Loading branch information
@michael-o
michael-o committed Oct 22, 2021
1 parent fb5f3f5 commit 5c36bf5
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 97 deletions.
102 changes: 48 additions & 54 deletions maven-core/src/main/java/org/apache/maven/project/MavenProject.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,13 @@
public class MavenProject
implements Cloneable
{

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

public static final String EMPTY_PROJECT_GROUP_ID = "unknown";

public static final String EMPTY_PROJECT_ARTIFACT_ID = "empty-project";

public static final String EMPTY_PROJECT_VERSION = "0";

private Model model;
Expand All @@ -107,6 +111,10 @@ public class MavenProject

private Set<Artifact> resolvedArtifacts;

private ArtifactFilter artifactFilter;

private Set<Artifact> artifacts;

private Artifact parentArtifact;

private Set<Artifact> pluginArtifacts;
Expand Down Expand Up @@ -143,7 +151,8 @@ public class MavenProject

private Artifact artifact;

private ThreadLocal<ArtifactsHolder> threadLocalArtifactsHolder = newThreadLocalArtifactsHolder();
// calculated.
private Map<String, Artifact> artifactMap;

private Model originalModel;

Expand Down Expand Up @@ -176,21 +185,12 @@ public class MavenProject
public MavenProject()
{
Model model = new Model();

model.setGroupId( EMPTY_PROJECT_GROUP_ID );
model.setArtifactId( EMPTY_PROJECT_ARTIFACT_ID );
model.setVersion( EMPTY_PROJECT_VERSION );
setModel( model );
}

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

public MavenProject( Model model )
Expand Down Expand Up @@ -695,11 +695,10 @@ public void addLicense( License license )

public void setArtifacts( Set<Artifact> artifacts )
{
ArtifactsHolder artifactsHolder = threadLocalArtifactsHolder.get();
artifactsHolder.artifacts = artifacts;
this.artifacts = artifacts;

// flush the calculated artifactMap
artifactsHolder.artifactMap = null;
artifactMap = null;
}

/**
Expand All @@ -712,36 +711,34 @@ public void setArtifacts( Set<Artifact> artifacts )
*/
public Set<Artifact> getArtifacts()
{
ArtifactsHolder artifactsHolder = threadLocalArtifactsHolder.get();
if ( artifactsHolder.artifacts == null )
if ( artifacts == null )
{
if ( artifactsHolder.artifactFilter == null || resolvedArtifacts == null )
if ( artifactFilter == null || resolvedArtifacts == null )
{
artifactsHolder.artifacts = new LinkedHashSet<>();
artifacts = new LinkedHashSet<>();
}
else
{
artifactsHolder.artifacts = new LinkedHashSet<>( resolvedArtifacts.size() * 2 );
artifacts = new LinkedHashSet<>( resolvedArtifacts.size() * 2 );
for ( Artifact artifact : resolvedArtifacts )
{
if ( artifactsHolder.artifactFilter.include( artifact ) )
if ( artifactFilter.include( artifact ) )
{
artifactsHolder.artifacts.add( artifact );
artifacts.add( artifact );
}
}
}
}
return artifactsHolder.artifacts;
return artifacts;
}

public Map<String, Artifact> getArtifactMap()
{
ArtifactsHolder artifactsHolder = threadLocalArtifactsHolder.get();
if ( artifactsHolder.artifactMap == null )
if ( artifactMap == null )
{
artifactsHolder.artifactMap = ArtifactUtils.artifactMapByVersionlessId( getArtifacts() );
artifactMap = ArtifactUtils.artifactMapByVersionlessId( getArtifacts() );
}
return artifactsHolder.artifactMap;
return artifactMap;
}

public void setPluginArtifacts( Set<Artifact> pluginArtifacts )
Expand Down Expand Up @@ -1186,8 +1183,7 @@ 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 @@ -1230,7 +1226,6 @@ 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 @@ -1239,6 +1234,11 @@ 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() );
Expand Down Expand Up @@ -1433,9 +1433,8 @@ public DependencyFilter getExtensionDependencyFilter()
public void setResolvedArtifacts( Set<Artifact> artifacts )
{
this.resolvedArtifacts = ( artifacts != null ) ? artifacts : Collections.<Artifact>emptySet();
ArtifactsHolder artifactsHolder = threadLocalArtifactsHolder.get();
artifactsHolder.artifacts = null;
artifactsHolder.artifactMap = null;
this.artifacts = null;
this.artifactMap = null;
}

/**
Expand All @@ -1448,10 +1447,9 @@ public void setResolvedArtifacts( Set<Artifact> artifacts )
*/
public void setArtifactFilter( ArtifactFilter artifactFilter )
{
ArtifactsHolder artifactsHolder = threadLocalArtifactsHolder.get();
artifactsHolder.artifactFilter = artifactFilter;
artifactsHolder.artifacts = null;
artifactsHolder.artifactMap = null;
this.artifactFilter = artifactFilter;
this.artifacts = null;
this.artifactMap = null;
}

/**
Expand Down Expand Up @@ -1481,7 +1479,13 @@ 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 @@ -1502,6 +1506,7 @@ public String getModulePathAdjustment( MavenProject moduleProject )
if ( moduleFile != null )
{
File moduleDir = moduleFile.getCanonicalFile().getParentFile();

module = moduleDir.getName();
}

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

reportArtifactMap = null;
}

Expand All @@ -1838,13 +1844,15 @@ public Map<String, Artifact> getReportArtifactMap()
{
reportArtifactMap = ArtifactUtils.artifactMapByVersionlessId( getReportArtifacts() );
}

return reportArtifactMap;
}

@Deprecated
public void setExtensionArtifacts( Set<Artifact> extensionArtifacts )
{
this.extensionArtifacts = extensionArtifacts;

extensionArtifactMap = null;
}

Expand All @@ -1861,6 +1869,7 @@ public Map<String, Artifact> getExtensionArtifactMap()
{
extensionArtifactMap = ArtifactUtils.artifactMapByVersionlessId( getExtensionArtifacts() );
}

return extensionArtifactMap;
}

Expand All @@ -1878,6 +1887,7 @@ 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 @@ -1981,20 +1991,4 @@ public void setProjectBuildingRequest( ProjectBuildingRequest projectBuildingReq
{
this.projectBuilderConfiguration = projectBuildingRequest;
}

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: 0 additions & 43 deletions maven-core/src/test/java/org/apache/maven/project/MavenProjectTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,13 @@

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 @@ -193,44 +188,6 @@ 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

0 comments on commit 5c36bf5

Please sign in to comment.