[MSHADE-467] Fix concurrency problem with dependency-reduced POM #210
Conversation
|
IT is in place now after a force-push. Please review. @stl543, please build the project with this PR locally and re-test, then report back. |
|
Hm, according to https://github.com/kriegaex/maven-shade-plugin/actions/runs/7592589521, the fix does not work universally on all combinations of Maven and JDK versions. I am open for input. |
It looks as if running MShade concurrently somehow bleeds MavenSession state into other mojo executions. Therefore, in the lower critical section of ShadeMojo::rewriteDependencyReducedPomIfWeHaveReduction, we use a final static ReentrantLock instance to block concurrent entry to the section.
|
OK, this build has passed now. For Maven 3.9.6, synchronisation on a single session instance was enough, but for Maven 3.6.3 it was a bit too naive and optimistic. Complex objects like project, session etc. contain lots of nested stuff, and not everything can or even should be deep-cloned as was done in MSHADE-413 in another part of the mojo. That can work, but leads to potentially brittle solutions, if the plugin needs to run on multiple Maven versions. So, protection of the critical section with a |
src/main/java/org/apache/maven/plugins/shade/mojo/ShadeMojo.java
Outdated
Show resolved
Hide resolved
Code review: Synchronising on session.getProjectBuildingRequest() is less invasive than a global reentrant lock and therefore preferable to the reviewer. Co-authored-by: Romain Manni-Bucau <rmannibucau@gmail.com>
|
Big big thank you for your work @kriegaex 👍 |
|
@rmannibucau, any idea why the build passed for the whole matrix on my fork, but here it failed due to locked files on Windows? |
|
I do not think it is pure chance that it happens in the new IT. Maybe, in some other part of Maven, at least in some versions, there are other concurrency issues we are hitting here by chance, just because we are testing this setup. If you run the build a few times and it does not re-occur, maybe the test is "just" flaky because of Maven or some situation on the GitHub server. If it happens more often, maybe we can try my initial solution with the reentrant lock and see if that one produces more stable results. Looking at the stack trace below the error message in the Maven 3.6.3 JDK 8 Windows build, it seems as if the Maven |
|
@kriegaex I build the reproducer and my project with the current master version with maven 3.8.6, 3.9.4 and 3.9.6 : Everything works fine ! |
|
@stl543, @fmarot, you are welcome. But looking at the commits, what effectively ended up in the merge were basically your reproducer and a variation of my fix provided by @rmannibucau. I.e., almost all the credit stays in France. I just debugged and connected the dots. Allez les Bleus! 🇫🇷 |
|
Resolve #721 |
1 similar comment
|
Resolve #721 |
6 participants
Fixes https://issues.apache.org/jira/browse/MSHADE-467.
You can review the fix. I converted the reproducer project into the integration test from the first commit. The second commit makes the previously failing test pass.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.