gnodet-bot commented on code in PR #12694:
URL: https://github.com/apache/maven/pull/12694#discussion_r4057956410
##########
impl/maven-core/src/main/java/org/apache/maven/project/MavenProject.java:
##########
@@ -898,13 +899,12 @@ public Set<Artifact> getArtifacts() {
if (artifactFilter == null || resolvedArtifacts == null) {
artifacts = new LinkedHashSet<>();
} else {
- Set<Artifact> result = new
LinkedHashSet<>(resolvedArtifacts.size() * 2);
+ artifacts = new LinkedHashSet<>(resolvedArtifacts.size() * 2);
for (Artifact artifact : resolvedArtifacts) {
if (artifactFilter.include(artifact)) {
- result.add(artifact);
+ artifacts.add(artifact);
}
Review Comment:
⚠️ **Concurrency regression — partially-populated `artifacts` visible to
concurrent readers.**
The old code built into a local `result` variable and then assigned
`artifacts = result` atomically as the last step. This squash eliminates the
local variable and writes directly into `artifacts` during the loop — so
concurrent callers of `getArtifacts()` can observe `artifacts != null` (because
it was assigned on line 903) but iterate a set that is only half-populated
(because the loop hasn't finished yet).
The deleted
`MavenProjectGetArtifactsTest.concurrentGetArtifactsDoesNotExposeAHalfBuiltSet()`
existed precisely to catch this race. Restoring the intermediate variable also
restores the test.
```suggestion
Set<Artifact> result = new
LinkedHashSet<>(resolvedArtifacts.size() * 2);
for (Artifact artifact : resolvedArtifacts) {
if (artifactFilter.include(artifact)) {
result.add(artifact);
}
}
artifacts = result;
```
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -560,28 +568,12 @@ private boolean upgradePropertyVersion(
return false; // Found in current POM, no upgrade needed
}
Review Comment:
⚠️ **Sibling-POM mutation regression — ancestor-directory guard removed.**
The deleted code (lines `-1093`..`-1112`) restricted the cross-POM property
search to POMs in ancestor directories:
```java
if (candidateDir == null || !currentDir.startsWith(candidateDir)) {
continue;
}
```
This was intentional: in a multi-module project, if child-A and child-B both
define property `maven.compiler.release` with sibling pom-A defining an old
version, the old code correctly skipped sibling-A when upgrading from child-B.
The new code iterates *all* POMs without that guard.
Concrete failure case: a project with `parent/pom.xml` defining
`${myPlugin.version}=3.0` and sibling `lib/pom.xml` defining
`${myPlugin.version}=3.9`. When processing `app/pom.xml` (a child of `parent`),
the code will now iterate both `lib/pom.xml` and `parent/pom.xml`, potentially
upgrading the property in `lib/pom.xml` — a sibling of `app`, not its parent.
That is incorrect.
The ancestor-directory filter must be restored.
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java:
##########
@@ -289,8 +294,11 @@ public UpgradeResult doApply(UpgradeContext context,
Map<Path, Document> pomMap)
}
}
+ // Clean up temp directory
+ cleanupTempDirectory(tempDir);
Review Comment:
⚠️ **Temp-directory leak when an exception is thrown.**
`cleanupTempDirectory(tempDir)` is inside the `try` block. If
`analyzePluginsUsingEffectiveModels()` or any code between line 230 and 298
throws an uncaught exception, execution jumps to the outer `catch (Exception
e)` block (line 300) and cleanup is never called. The temp directory in `/tmp`
is leaked for the remainder of the JVM lifetime.
Fix: use `try/finally`, or capture the path before the try and clean up in
the catch:
```suggestion
} finally {
cleanupTempDirectory(tempDir);
}
```
(Move the `cleanupTempDirectory` call into a `finally` block wrapping the
inner logic, not the outer catch.)
--
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]
