[MNG-6843] Thread-safe artifacts in MavenProject #413

famod · 2020-12-20T21:30:42Z

Avoids concurrency issues when aggregator plugins are involved.
A detailed summary of the problem can be found here: #310 (comment)

Might also fix:

This is an alternative to #310 in which @rfscholte raised concerns whether cloning is the right approach.

I went with a wrapper and a single ThreadLocal because mutliple TLs would bloat the code and would theoretically increase overhead (haven't measured it, though).
I had to be creative with removing empty lines because Checkstyle was complaining about the class exceeding 2000 lines.

PS: I removed the checklist to avoid noise (checked everything, ICLA is present).

famod · 2020-12-20T22:20:44Z

I'm also getting this MavenITmng3259DepsDroppedInMultiModuleBuildTest failure on JDK16 locally on master.
So I don't think my changes have anything to do with it.

michael-o · 2020-12-20T22:26:56Z

What error message do you get?

famod · 2020-12-20T22:30:28Z

@michael-o

Exit code was non-zero: 1
...
[INFO] --- maven-surefire-plugin:2.3:test (default-test) @ mng-module5-XXX ---
[INFO] Surefire report directory: C:\_dev\git\maven-integration-testing\core-it-suite\target\test-classes\mng-3259\module5\target\surefire-reports

-------------------------------------------------------
 T E S T S
-------------------------------------------------------
Running mng.Module5Test
Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.056 sec <<< FAILURE!
testJMockAvailable(mng.Module5Test)  Time elapsed: 0.044 sec  <<< ERROR!
java.lang.ExceptionInInitializerError
	at com.thoughtworks.xstream.XStream.setupConverters(XStream.java:584)
	at com.thoughtworks.xstream.XStream.<init>(XStream.java:375)
	at com.thoughtworks.xstream.XStream.<init>(XStream.java:301)
	at mng.XStreamTestCase.setUp(XStreamTestCase.java:13)
	at org.jmock.core.VerifyingTestCase.runBare(VerifyingTestCase.java:37)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:120)
	at junit.framework.TestSuite.runTest(TestSuite.java:230)
	at junit.framework.TestSuite.run(TestSuite.java:225)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at org.apache.maven.surefire.junit.JUnitTestSet.execute(JUnitTestSet.java:213)
	at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.executeTestSet(AbstractDirectoryTestSuite.java:138)
	at org.apache.maven.surefire.suite.AbstractDirectoryTestSuite.execute(AbstractDirectoryTestSuite.java:125)
	at org.apache.maven.surefire.Surefire.run(Surefire.java:132)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at org.apache.maven.surefire.booter.SurefireBooter.runSuitesInProcess(SurefireBooter.java:290)
	at org.apache.maven.surefire.booter.SurefireBooter.main(SurefireBooter.java:818)
Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make field protected volatile java.util.Properties java.util.Properties.defaults accessible: module java.base does not "opens java.util" to unnamed module @5f205aa
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
	at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
	at com.thoughtworks.xstream.core.util.Fields.find(Fields.java:15)
	at com.thoughtworks.xstream.converters.collections.PropertiesConverter.<clinit>(PropertiesConverter.java:29)
	... 25 more

CI looks the same, AFAICT.

michael-o · 2020-12-21T08:58:46Z

This likes needs an XStream update...

famod · 2020-12-22T00:04:49Z

@michael-o How come other PRs don't show this failure? I also had a brief look at your Jenkins jobs and I couldn't find anything there either (but I may have looked in the wrong place).

I have a fix, simply updating xstream to 1.4.15 in core-it-suite/src/test/resources/mng-3259/parent/pom.xml does the trick.
~~Do you want a separate PR for that?~~ Edit: Of course you do, it's a separate repo. 🤦 If yes, I hope I don't need a MNG ticket...?

michael-o · 2020-12-22T08:20:17Z

PR is fine.

Avoids concurrency issues when aggregator plugins are involved.

famod · 2020-12-22T11:37:22Z

All green! 🚀

famod · 2021-01-05T14:13:07Z

@michael-o So how do we move on with this? Who needs to approve?

michael-o · 2021-01-05T14:14:47Z

I would at least have @rfscholte and @khmarbaise look over. @famod Do you think an IT is possible here?

famod · 2021-01-05T14:21:05Z

Thanks for your quick answer.

Do you think an IT is possible here?

One that is trying to provoke the concurrency issue? I fear such a test would be rather flaky...

michael-o · 2021-01-05T14:22:54Z

I guess so, that's the problem. What if one writes a sleep mojo?

famod · 2021-01-05T14:30:57Z

That could work, with a serious amount of effort though. Are there any exiting ITs that come with their own custom mojos?

michael-o · 2021-01-05T14:46:58Z

Yes, there are. I need search because I have touched one recently.

famod · 2021-01-17T18:23:03Z

@rfscholte Thanks!

@michael-o

I need search because I have touched one recently.

Can you point me to such a test with a custom mojo? I would then try to take a (timeboxed) stab at it.
Thanks!

famod · 2021-01-26T10:31:18Z

@michael-o friendly reminder:

Can you point me to such a test with a custom mojo? I would then try to take a (timeboxed) stab at it.
Thanks!

michael-o · 2021-01-26T12:55:52Z

Please ping me next week, I am current incapable to do anything fruitful. Alternatively, go through Core ITs commits from me and check Plugin changes.

gnodet · 2021-05-12T20:38:15Z

@famod In your experiments, is there any case where the output of the computation of the artifacts list is not always the same for a given project ? I mean, why choosing a TLS rather than simply adding a synchronized on the getArtifacts() method (or any fine grained synchronization fwiw) ?

Tibor17 · 2021-05-12T22:41:57Z

Why we do it so complicated? The threads do not share their objects with ThreadLocal-s. So every Thread will see another copy.
If I was fixing this issue, I would use thread-safe collections and synchronized critical section setArtifacts() because first you have to clear the collection and then set new content in the critical section. That's it.
But I am convinced that the issues in Maven are due to Java Memory Model that objects in ArrayList are weakly consistent. So you have to use the COWAL collection instead. Example, even if you use any treatment in this class, you pass a reference of the Set to the setter setArtifacts() you still may have a problem because the caller thread used thread-unsafe Set. So the caller must create ideally immutable Set or thread safe Set in order to copy the reference. If a copy of the content of Set is acceptable then refuse copying the reference and the caller can be any thread unsafe object.

famod · 2021-05-14T15:54:37Z

@gnodet @Tibor17 Have you guys read #310 (comment)? (and maybe also #310 (comment))?
The main problem is here:

maven/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java

Lines 280 to 283 in c3cf294

 for ( MavenProject projectToResolve : projectsToResolve ) 

 { 

 projectToResolve.setArtifactFilter( artifactFilter ); 

 }

I don't see how a fine grained synchronization or a COWAL in MavenProject can help here. The execution of an aggregation goal would have to lock all other invocations of getArtifacts() etc. (however those may look like) until done with the artifact processing.

Overhauling the (problematic) design of aggregation goal handling would be the most sustainable solution. But neither me nor the others that took a stab at this have the knowledge or capacity to do that.

But even then: MavenProject is highly mutable, there are setters everywhere (which is good for Maven extensions that enhance the Maven experience but often bad for concurrency stability).

gnodet · 2021-05-14T18:48:51Z

#310 (comment)

Ah, got it, thx.

Tibor17 · 2021-05-15T20:07:21Z

@famod
Many people thing that the problem is always about writes. Wrong! The concurrency is also about reads, which you did not show me! It is not about happens before relationship in your code because it is about this theorem in the memory and you never know when the (internal index: int, array: [] of ArrayList) appears in CPU registers and cache (Java working copy), or RAM (Java main shared memory). If you use synchronization on MavenProject's methods or thread safe collections, you know that the happens before theorem works as always, and you can just sleep well.

gnodet · 2021-05-15T20:27:26Z

@Tibor17 this is not a memory model problem here. The analysis done by @famod shows that when using a parallel build, multiple mojos may use the fields on the MavenProject concurrently but with different values (the artifactFilter may be different for each mojo, hence the resulting artifacts list). So synchronizing on those variables would not help, as one mojo may write a value but later read a non coherent artifacts list. The problem here is that the artifactFilter and artifacts are specific to a given mojo execution.

Tibor17 · 2021-05-15T20:34:04Z

Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.

gnodet · 2021-05-15T20:34:54Z

Then the usage of ThreadLocal is still wrong because one thread may see multiple Mojos. If the filter should be different for each Mojo, then it is design problem. Why to handle Mojo's variables outside? Maybe some Memento object should be apart of MavenProject.

How could that happen ? A thread can not execute multiple mojo concurrently...
And yes, there's a design problem, but this PR is about fixing a bug imho, not changing the design of the core maven API.

Tibor17 · 2021-05-15T20:36:27Z

but for me, a fix means removing the design problem

gnodet · 2021-05-15T20:39:03Z

That

but for me, a fix means removing the design problem

A fix would require to remove the MavenProject.getArtifacts() method somehow. It's part of the core mojo API, so not sure how that can be done in a short time frame. In the mean time, any acceptable bug to be able to use parallel builds in a more robust way would be handy...

Tibor17 · 2021-05-16T13:10:05Z

@famod
@gnodet
Speaking for myself, I would propose the following

deprecate the method getArtifacts(), and
introduce a new method getArtifacts( ArtifactFilter artifactFilter )

It looks to me that the plugin itself is the owner of the object ArtifactFilter and not the object MavenProject. WDYT?

gnodet · 2021-05-16T16:41:47Z

@famod
@gnodet
Speaking for myself, I would propose the following

deprecate the method getArtifacts(), and

introduce a new method getArtifacts( ArtifactFilter artifactFilter )

It looks to me that the plugin itself is the owner of the object ArtifactFilter and not the object MavenProject. WDYT?

That was my initial thought too. But I think it does not really work, as the ArtifactFilter is not known directly by the mojo being executed (it's constructed by maven internals through mojo annotations / javadoc tags iirc) and the call sites for getArtifacts() usually have no knowledge of the mojo being executed afaik...

gnodet · 2021-05-17T06:33:02Z

@famod
@gnodet
Speaking for myself, I would propose the following

deprecate the method getArtifacts(), and

introduce a new method getArtifacts( ArtifactFilter artifactFilter )

It looks to me that the plugin itself is the owner of the object ArtifactFilter and not the object MavenProject. WDYT?

That was my initial thought too. But I think it does not really work, as the ArtifactFilter is not known directly by the mojo being executed (it's constructed by maven internals through mojo annotations / javadoc tags iirc) and the call sites for getArtifacts() usually have no knowledge of the mojo being executed afaik...

I may be wrong about the call sites, they may be inside the mojos most of the cases.
One possibility could be to inject the ArtifactFilter into mojos, in which case, they could easily replace the getArtifacts() or similar calls with getArtifacts(artifactFilter). Other methods like getCompileClasspathElements(), getTestClasspathElements(), getRuntimeClasspathElements() will need some investigation too.

gnodet · 2021-06-02T08:01:43Z

@Tibor17 See #475

michael-o · 2021-06-25T08:12:47Z

Will check this night.

rfscholte · 2021-06-25T17:30:44Z

Merged with 73e00ed

famod mentioned this pull request Dec 20, 2020

[MNG-6843] - Parallel build fails due to missing JAR artifacts in compilePath #310

Closed

8 tasks

famod mentioned this pull request Dec 22, 2020

Fix mng-3259 test on JDK 16 apache/maven-integration-testing#89

Closed

[MNG-6843] Thread-safe artifacts in MavenProject

6aa1fb3

Avoids concurrency issues when aggregator plugins are involved.

famod force-pushed the MNG-6843-parallel-classpath branch from 2eb5fa2 to 6aa1fb3 Compare December 22, 2020 11:04

rfscholte approved these changes Jan 17, 2021

View reviewed changes

lanmaoxinqing mentioned this pull request Jun 1, 2021

Parallel build fails due to missing JAR artifacts in compilePath apache/maven-mvnd#418

Closed

gnodet approved these changes Jun 1, 2021

View reviewed changes

gnodet mentioned this pull request Jun 3, 2021

[MNG-7157] [MNG-6843] Provide a better design for obtaining artifacts… #475

Open

rfscholte closed this Jun 25, 2021

famod deleted the MNG-6843-parallel-classpath branch June 26, 2021 19:47

famod mentioned this pull request Jun 26, 2021

[MNG-6843] [3.8.x] Thread-safe artifacts in MavenProject #482

Closed

famod mentioned this pull request Aug 22, 2021

prefer use of InheritableThreadLocal to fix possible issue when mojo start a thread and try to access some artifacts #521

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-6843] Thread-safe artifacts in MavenProject #413

[MNG-6843] Thread-safe artifacts in MavenProject #413

famod commented Dec 20, 2020 •

edited

Loading

famod commented Dec 20, 2020

michael-o commented Dec 20, 2020

famod commented Dec 20, 2020

michael-o commented Dec 21, 2020

famod commented Dec 22, 2020 •

edited

Loading

michael-o commented Dec 22, 2020

famod commented Dec 22, 2020 •

edited

Loading

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 17, 2021

famod commented Jan 26, 2021

michael-o commented Jan 26, 2021

gnodet commented May 12, 2021

Tibor17 commented May 12, 2021 •

edited

Loading

famod commented May 14, 2021 •

edited

Loading

gnodet commented May 14, 2021

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021 •

edited

Loading

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021 •

edited

Loading

Tibor17 commented May 16, 2021

gnodet commented May 16, 2021 •

edited

Loading

gnodet commented May 17, 2021

gnodet commented Jun 2, 2021

michael-o commented Jun 25, 2021

rfscholte commented Jun 25, 2021

[MNG-6843] Thread-safe artifacts in MavenProject #413

[MNG-6843] Thread-safe artifacts in MavenProject #413

Conversation

famod commented Dec 20, 2020 • edited Loading

famod commented Dec 20, 2020

michael-o commented Dec 20, 2020

famod commented Dec 20, 2020

michael-o commented Dec 21, 2020

famod commented Dec 22, 2020 • edited Loading

michael-o commented Dec 22, 2020

famod commented Dec 22, 2020 • edited Loading

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 5, 2021

michael-o commented Jan 5, 2021

famod commented Jan 17, 2021

famod commented Jan 26, 2021

michael-o commented Jan 26, 2021

gnodet commented May 12, 2021

Tibor17 commented May 12, 2021 • edited Loading

famod commented May 14, 2021 • edited Loading

gnodet commented May 14, 2021

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021 • edited Loading

Tibor17 commented May 15, 2021

gnodet commented May 15, 2021 • edited Loading

Tibor17 commented May 16, 2021

gnodet commented May 16, 2021 • edited Loading

gnodet commented May 17, 2021

gnodet commented Jun 2, 2021

michael-o commented Jun 25, 2021

rfscholte commented Jun 25, 2021

famod commented Dec 20, 2020 •

edited

Loading

famod commented Dec 22, 2020 •

edited

Loading

famod commented Dec 22, 2020 •

edited

Loading

Tibor17 commented May 12, 2021 •

edited

Loading

famod commented May 14, 2021 •

edited

Loading

gnodet commented May 15, 2021 •

edited

Loading

gnodet commented May 15, 2021 •

edited

Loading

gnodet commented May 16, 2021 •

edited

Loading