gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4059230773
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultSession.java:
##########
@@ -172,6 +175,15 @@ public int getDegreeOfConcurrency() {
return getMavenSession().getRequest().getDegreeOfConcurrency();
}
+ @Nonnull
+ @Override
+ public BuildEnvironment buildEnvironment() {
+ if (buildEnvironment == null) {
Review Comment:
**[Medium, RERAISED] Non-atomic lazy init on a `@ThreadSafe` class —
`volatile` alone is not enough.**
`Session` is annotated `@ThreadSafe`, meaning two threads can legally call
`buildEnvironment()` concurrently. With the current pattern:
```java
if (buildEnvironment == null) { // T1 and T2 both see null
buildEnvironment = BuildReportCollector.buildEnvironment(...); // both
execute
}
```
both threads can observe `null` and both call `buildEnvironment(session)`,
which iterates user properties (redacting sensitive keys), reads system
properties, and allocates `LinkedHashMap`/`TreeMap`. The `volatile` guarantees
visibility of the final write but not atomicity of the check-then-act compound
action. Use double-checked locking:
```suggestion
if (buildEnvironment == null) {
synchronized (this) {
if (buildEnvironment == null) {
buildEnvironment =
BuildReportCollector.buildEnvironment(getMavenSession());
}
}
}
```
Or `AtomicReference.compareAndSet` if you prefer lock-free. This violates
the `@ThreadSafe` contract semantically (two threads can each construct a
separate `DefaultBuildEnvironment` reading independent property snapshots).
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/standalone/ApiRunner.java:
##########
@@ -381,6 +382,78 @@ public int getDegreeOfConcurrency() {
return 0;
}
+ @Override
+ public BuildEnvironment buildEnvironment() {
+ // ApiRunner is a standalone/embedded session with no
MavenExecutionRequest;
+ // return a minimal environment reflecting defaults.
+ return new BuildEnvironment() {
+ @Override
+ public List<String> goals() {
+ return List.of();
+ }
+
+ @Override
+ public Map<String, String> userProperties() {
+ return Map.of();
+ }
+
+ @Override
+ public Map<String, String> systemInfo() {
+ return Map.of();
+ }
+
+ @Override
+ public String localRepository() {
+ return "";
+ }
+
+ @Override
+ public List<String> activeProfiles() {
+ return List.of();
+ }
+
+ @Override
+ public List<String> selectedProjects() {
+ return List.of();
+ }
+
+ @Override
+ public String resumeFrom() {
+ return null;
+ }
+
+ @Override
+ public String reactorFailureBehavior() {
+ return "FAIL_FAST";
+ }
+
+ @Override
+ public boolean offline() {
+ return false;
+ }
+
+ @Override
+ public boolean updateSnapshots() {
+ return false;
+ }
+
+ @Override
+ public boolean noTransferProgress() {
+ return false;
+ }
+
+ @Override
+ public boolean batchMode() {
+ return false;
+ }
+
+ @Override
+ public int threads() {
+ return 1;
+ }
+ };
Review Comment:
**[Low, RERAISED] Anonymous `BuildEnvironment` — silent API drift risk.**
`maven-impl` cannot depend on `maven-core` (where `DefaultBuildEnvironment`
lives), so using the concrete class directly is off the table. But a raw
anonymous class has a silent failure mode: if a new non-`default` abstract
method is added to `BuildEnvironment` in the future, this anonymous class will
fail to compile with no contextual indication that the `ApiRunner` path needs
updating.
A cleaner alternative: extract a package-private `MinimalBuildEnvironment`
(or similar) into `maven-api-core` alongside the interface, explicitly
implementing the "all-defaults" contract. Any new interface addition
immediately becomes a compilation gap there.
At minimum, add a maintenance comment:
```java
// NOTE: If BuildEnvironment gains new abstract methods, update this
anonymous class.
// DefaultBuildEnvironment (maven-core) cannot be used here — module
boundary.
```
Low severity given `@Experimental`, but worth addressing before
stabilization.
--
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]
