Fix #354 Make artifact cache safe for concurrent access

[Stephan202]

No description provided.

[bjanczak]

This fix resolves issue only partially. There is similar issue in DependenciesTool.java line 118.

[lama0206]

@bjanczak Why's there a problem in DependenciesTool? The localCache is imho tread-local and thus there's no concurrent access. The cache is read from the cache parameter/variable coming from the invocation in DefaultThirdPartyHelper which is thread-safe given this PR.

[lama0206]

I'd add double-checked locking:

wrap 142 like this:
synchronized(this); if(artifactCache==null) {
--> initalization here
}

[Stephan202]

@lama0206 interesting idea. Turns out artifactCache is static (yeah...), so synchronizing on this may not be enough. Could do this:

Suggested change

	artifactCache = Collections.synchronizedSortedMap( new TreeMap<String, MavenProject>() );
	synchronized (DefaultThirdPartyHelper.class)
	{
	if ( artifactCache == null )
	{
	artifactCache = Collections.synchronizedSortedMap(new TreeMap<String, MavenProject>());
	}
	}

WDYT?

(It's been a long while since I looked at this code, so not sure whether this field should be static. Can try to re-familiarize myself with the code later; likely not this evening.)

[lama0206]

Yep, @Stephan202, that should be it. Thanks for double-checking :)

Concerning static: #398 discusses the artifactCache being static (and never released/cleaned) and thus eating memory for the entire mvn build. I think this issue (#354) is about correctness and #398 is about memory consumption. So I'd say that this here is fine.

[Stephan202]

Alright, rebased and updated the commit (squashed to keep history clean, since it appears in this repo squash-and-rebase isn't used much/at all).

Locally the ISSUE-324/pom.xml integration test fails, but that also happens on master. I was gonna say "let's see what happens on Travis CI", but it seems this repository hasn't been migrated to travis-ci.com yet, while travis-ci.org has ceased operations. Luckily there's also an AppVeyor build.)

[Stephan202]

Rebased. Would still be nice to get this PR merged :)

[frant-hartm]

Hi,

we are occasionally facing the #354 issue with the plugin when running a parallel build.

I have looked into @Stephan202's fix and I think additional modifications are needed:

• the field artifactCache must be volatile to prevent partially constructed object being used

• static private mutex object should be used instead of SynchronizedMap.class, one never knows who else might be synchronizing on that class

• All uses of artifactCache instance where it is being iterated over the elements must be manually synchronized, e.g. if you look at the following line

license-maven-plugin/src/main/java/org/codehaus/mojo/license/api/DependenciesTool.java

Line 120 in 5fb1c23

localCache.putAll( cache );

it must be in a synchronized block on cache instance to prevent the concurrent modification failure. See the javadoc of synchronizedSortedMap

I believe these changes make the AddThirdPartyMojo thread-safe, so it can be marked as such. I didn't investigate the AggregatorThirdPartyReportMojo apart from the uses of the artifactCache.

I have made these changes available here https://github.com/frant-hartm/license-maven-plugin/commits/bugfix/issue-354

@Stephan202 you can either add them to this PR or I can send a separate PR with everything.

[Stephan202]

@frant-hartm thanks for breathing some life into this PR! You're totally right that the synchronized (SortedMap.class) construct was dodgy; thanks for calling that out :)

I cherry-picked your changes to the PR of this branch so that we can collaborate on them here. That said, I can totally understand if you eventually want to open a separate PR with these changes.

[Stephan202]

@frant-hartm I sent you an invite to be an outside collaborator on the Picnic fork; that might simplify collaboration :)

[Stephan202]

Alright, I had my first coffee and refreshed my memory about this PR by reading the Collections.synchronizedSortedMap(java.util.SortedMap) documentation. I traced the how the artifact cache is passed around, and I think all iteration operations are now synchronized. 💪

(This setup is still very fragile, since future modification can easily forget to add additional synchronization if necessary, but fixing that requires (much) larger changes.)

[Stephan202]

NB: Here and further down the null check seems redundant, since IIUC callers never pass in null. (But we might not want to change this, in order to keep the PR minimal.)

[Stephan202]

I was reminded of this PR because the issue it attempts to fix happened again.

@bjanczak @lama0206 @frant-hartm WDYT of the current state of the PR, and/or do you happen to know how we could get this merged and released?

[frant-hartm]

There are multiple issues with the current master - mvnw script is broken, failing tests, ...

Those probably need to be fixed first before merging any bugfixes.

Maybe someone who committed to master during the last year could help? @slawekjaranowski or @VonUniGE @slachiewicz @tpage-alfresco

[frant-hartm]

@olamy I see that you have released 2.0.1 recently, thanks a lot for your work.
Is there any way I can help to get this merged and released?

I would like to finally get rid of the following:

[WARNING] *****************************************************************
[WARNING] * Your build is requesting parallel execution, but this         *
[WARNING] * project contains the following plugin(s) that have goals not  *
[WARNING] * marked as thread-safe to support parallel execution.          *
[WARNING] * While this /may/ work fine, please look for plugin updates    *
[WARNING] * and/or request plugins be made thread-safe.                   *
[WARNING] * If reporting an issue, report it against the plugin in        *
[WARNING] * question, not against Apache Maven.                           *
[WARNING] *****************************************************************
[WARNING] The following plugins are not marked as thread-safe in Hazelcast Root:
[WARNING]   org.codehaus.mojo:license-maven-plugin:2.0.0
[WARNING] 
[WARNING] Enable debug to see precisely which goals are not marked as thread-safe.
[WARNING] *****************************************************************

and a flaky concurrent modification exception in our build :-)