dashorst opened a new pull request, #13592: URL: https://github.com/apache/maven/pull/13592
Fixes #13590. ## Problem The concurrent builder runs independent steps of one project in parallel, for example `process-resources` next to `compile`. Before a mojo runs, `MojoExecutor.ensureDependenciesAreResolved` resolves the dependencies for the scopes of the mojo's *step* and stores them on the shared `MavenProject` (`setResolvedArtifacts`, `setArtifactFilter`). The mojo reads them back while it executes. The base `MojoExecutor` guards this with a per-project lock. The concurrent `MojoExecutor` turns that lock off (`useProjectLock` returns `false`), so steps of the same project overwrite each other's state. Symptoms in a 67-module build with `-b concurrent -T 1C`: - `compile` gets a classpath without its `provided` dependencies, and sometimes without compile-scoped ones; - `surefire:test` fails with "groups/excludedGroups require TestNG, JUnit48+ or JUnit 5 (a specific engine required on classpath)". The reproducer in #13590 fails on every run with two or more threads. Locking only the mojos that declare `requiresDependencyResolution`/`requiresDependencyCollection` is not enough. Resolution happens for the union of the scopes of all mojos in a step, so the first mojo of a step, for instance `resources:resources`, also writes the project's dependency state, and sets the artifact filter to `null`. A first version of this change that locked only dependency-hungry mojos still failed with the surefire symptom above. ## Change `concurrent.MojoExecutor` overrides `getProjectLock` and takes a `ReentrantLock` per project, kept in the session data, around dependency resolution and mojo execution. Unlike the base `ProjectLock`, it does not log a warning when it has to wait, because waiting for another step of the same project is expected here. Executions on different projects keep running in parallel. ## Trade-off The lock serializes the steps of one project, so a long `surefire:test` now also holds up, for example, `jar:jar` of the same project. Measured on the 67-module build, `clean verify -DskipITs -T 1C -b concurrent`, 32 cores: | | wall time | |---|---| | Maven 3.9.16, default builder | 7:38 | | 4.0.x without this change, with a workaround (flatten bound after `compile`) | 5:26, 5:35, but only correct by luck | | 4.0.x with this change, without workaround | 6:44, 7:56 | A finer alternative would be to give every step its own view of the resolved artifacts and the artifact filter on `MavenProject`, for example a thread-bound state installed around a step's mojo executions, so that steps no longer have to wait for each other. That changes `MavenProject`, which plugins use directly, so I kept this PR to the lock and would like your view on whether that direction is worth pursuing. ## Testing - New `MojoExecutorTest`: two executions on the same project do not overlap (also when one mojo has no dependency requirements), and executions on different projects do. Both locking cases fail without the change. - The reproducer from #13590 passes 5/5 with `-T 16`, against 5/5 failures before. - The 67-module build passes with `-b concurrent -T 1C`: three times without tests, and twice with all 801 test classes, without the workaround. - `mvn verify -pl impl/maven-core -am`: all 751 tests of maven-core pass. In my environment `DefaultModelBuilderTest` and `DefaultModelResolverTest` in maven-impl fail, on `master` without this change as well, so I ran with `-Dmaven.test.failure.ignore=true` for the upstream modules. Following this checklist to help us incorporate your contribution quickly and easily: - [x] Your pull request should address just one issue, without pulling in other changes. - [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why. - [x] Each commit in the pull request should have a meaningful subject line and body. - [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. - [ ] Run `mvn verify` to make sure basic checks pass. (Only `-pl impl/maven-core -am`, see Testing.) - [ ] You have run the [Core IT][core-its] successfully. - [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0) [core-its]: https://maven.apache.org/core-its/core-it-suite/ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
