gnodet-bot commented on code in PR #13591:
URL: https://github.com/apache/maven/pull/13591#discussion_r4334520857
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/concurrent/MojoExecutor.java:
##########
@@ -52,8 +58,57 @@ public MojoExecutor(
messageBuilderFactory);
}
+ /**
+ * The concurrent builder runs independent steps of the same project in
parallel, for instance
+ * {@code process-resources} next to {@code compile}. Before a mojo runs,
+ * {@link #ensureDependenciesAreResolved} replaces the resolved artifacts
and the artifact filter
+ * on the shared {@link MavenProject}, and the mojo reads them back while
it executes.
+ *
+ * <p>This is done for every legacy (non-v4) mojo, including the ones that
need no dependency:
+ * a {@code null} filter clears whatever an earlier mojo left, so that a
mojo with no requirement
+ * sees no project dependencies rather than another mojo's leftover ones
(MNG-3297). The executions
+ * of a single project are therefore serialized for legacy mojos.
+ *
+ * <p>Maven 4 mojos using {@code @Resolution} get their dependencies
injected directly by
+ * {@code DefaultMavenPluginManager} and never read from {@link
MavenProject}, so they skip both
+ * {@code ensureDependenciesAreResolved} and this lock. Executions on
different projects are
+ * never serialized.
+ */
@Override
- protected boolean useProjectLock(MavenSession session) {
- return false;
+ protected NoExceptionCloseable createProjectLock(MavenSession session,
MojoDescriptor mojoDescriptor) {
+ if (mojoDescriptor.isV4Api()) {
+ return NO_LOCK;
+ }
+ return new ConcurrentProjectLock(session, mojoDescriptor);
+ }
+
+ protected class ConcurrentProjectLock implements NoExceptionCloseable {
+
+ final OwnerReentrantLock lock;
+
+ ConcurrentProjectLock(MavenSession session, MojoDescriptor
mojoDescriptor) {
+ SessionData data = session.getSession().getData();
+ Map<MavenProject, OwnerReentrantLock> locks =
data.computeIfAbsent(PROJECT_LOCKS, ConcurrentHashMap::new);
+ MavenProject currentProject = session.getCurrentProject();
+ lock = locks.computeIfAbsent(currentProject, p -> new
OwnerReentrantLock());
+ if (!lock.tryLock()) {
+ Thread owner = lock.getOwner();
+ MojoDescriptor ownerMojo = owner != null ? mojos.get(owner) :
null;
+ String str = ownerMojo != null ? " The " + ownerMojo.getId() :
"A";
+ String msg = str
+ + " legacy (Maven 3) mojo is preventing full
intra-project parallelism in the concurrent builder "
+ + "on the project " + currentProject.getGroupId()
+ + ":" + currentProject.getArtifactId() + ". "
+ + "This mojo execution will be blocked until the mojo
is done. "
+ + "Consider migrating plugins to the Maven 4 API to
benefit from full intra-project concurrency.";
+ warn(msg);
Review Comment:
⚠️ **Warning not actionable for end users**: This fires on every v3 mojo
contention and tells users to "migrate plugins to the Maven 4 API" — but they
typically can't migrate third-party plugins. Per Maven convention, `WARN`
should be reserved for conditions the user can actually act on. This
serialization is expected, correct behavior for legacy mojos, not an anomaly
worth warning about.
Consider `LOGGER.debug()` here, or `LOGGER.info()` capped to the first
occurrence per project (e.g. track warned projects in a `Set<MavenProject>` in
`SessionData`).
```suggestion
LOGGER.debug(msg);
```
##########
impl/maven-core/src/main/java/org/apache/maven/lifecycle/internal/MojoExecutor.java:
##########
@@ -320,14 +321,27 @@ protected interface NoExceptionCloseable extends
AutoCloseable {
void close();
}
- protected NoExceptionCloseable getProjectLock(MavenSession session,
MojoDescriptor mojoDescriptor) {
+ public NoExceptionCloseable getProjectLock(MavenSession session,
MojoDescriptor mojoDescriptor) {
Review Comment:
💡 `protected` → `public` is unnecessary here. The test (`MojoExecutorTest`)
calls this on a `concurrent.MojoExecutor` instance, which is a subclass. In
Java, a `protected` method on the base class is accessible from subclass
references — the `concurrent` package test works fine with `protected`.
Widening to `public` expands the API surface of a class that explicitly says it
is "not part of any public api."
```suggestion
protected NoExceptionCloseable getProjectLock(MavenSession session,
MojoDescriptor mojoDescriptor) {
```
--
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]
