gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4059221488
##########
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] 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`. While the two results are
equivalent (no mutation), this is a double-computation that violates the
`@ThreadSafe` contract semantically.
Use double-checked locking:
```suggestion
if (buildEnvironment == null) {
synchronized (this) {
if (buildEnvironment == null) {
buildEnvironment =
BuildReportCollector.buildEnvironment(getMavenSession());
}
}
}
```
Or use `AtomicReference.compareAndSet` if you prefer lock-free. The
`volatile` write alone is safe for visibility, but the check-then-act compound
action needs synchronization.
##########
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;
+ }
+ };
+ }
+
@Override
public Instant getStartTime() {
Review Comment:
**[Low] Anonymous `BuildEnvironment` in `ApiRunner` — 60-line maintenance
burden with silent API drift risk.**
`maven-impl` can't depend on `maven-core` (where `DefaultBuildEnvironment`
lives), so using the record directly is off the table. But returning a raw
anonymous class has a silent failure mode: if a new non-default method is added
to `BuildEnvironment` in the future, this anonymous class will silently inherit
the interface default (if any), or fail to compile if it's abstract — with no
indication at the call site that the `ApiRunner` path needs updating.
A cleaner alternative: extract a package-private `MinimalBuildEnvironment`
(or equivalent) into `maven-api-core` alongside the `BuildEnvironment`
interface, implementing the "all defaults" contract explicitly. That way new
additions to the interface are immediately visible as a compilation gap.
Alternatively, document the current approach with a comment:
```java
// NOTE: If BuildEnvironment gains new methods, update this anonymous class.
// DefaultBuildEnvironment (maven-core) cannot be used here — module
boundary.
```
This is low-severity — the annotation `@Experimental` gives latitude — 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]
