gnodet-bot commented on code in PR #12695:
URL: https://github.com/apache/maven/pull/12695#discussion_r4059326839
##########
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) {
+ buildEnvironment =
BuildReportCollector.buildEnvironment(getMavenSession());
+ }
+ return buildEnvironment;
Review Comment:
**[Medium, RERAISED] Non-atomic lazy init on a `@ThreadSafe` object.**
```java
if (buildEnvironment == null) { // (1) thread A sees null
buildEnvironment = BuildReportCollector.buildEnvironment(...); // (2)
thread A allocates
} // (3) thread B also saw null at
(1), allocates again
return buildEnvironment;
```
`Session` is annotated `@ThreadSafe`. The `volatile` keyword prevents stale
reads but does NOT make the check-then-act atomic. Two threads in a parallel
build (`-T4`) can both observe `null`, both call
`BuildReportCollector.buildEnvironment()`, and both write. The result is two
distinct `DefaultBuildEnvironment` instances allocated and one silently
discarded — functionally harmless here (idempotent computation) but a
correctness violation of the `@ThreadSafe` contract, and a latent hazard if the
method ever gains side effects.
Fix with `synchronized` or a double-checked lock:
```suggestion
public BuildEnvironment buildEnvironment() {
BuildEnvironment env = this.buildEnvironment;
if (env == null) {
synchronized (this) {
env = this.buildEnvironment;
if (env == null) {
env =
BuildReportCollector.buildEnvironment(getMavenSession());
this.buildEnvironment = env;
}
}
}
return env;
}
```
Alternatively, populate the field in the constructor (since
`getMavenSession()` is available there) and make it `final`, which eliminates
the race entirely.
##########
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:
**[Medium, RERAISED] Anonymous `BuildEnvironment` re-instantiated on every
call — silent API drift risk.**
`buildEnvironment()` constructs and returns a new anonymous
`BuildEnvironment` class on every invocation. Two problems:
1. **Allocation churn**: embedded/daemon embedders that call
`session.buildEnvironment()` repeatedly pay a fresh object allocation each
time. Minor, but unnecessary.
2. **Silent API drift**: when `BuildEnvironment` gains a new method (it's
`@Experimental`), the anonymous class will fail to compile — but only here in
`ApiRunner`. Unlike a named class, there's no obvious place to look, and the
compiler error points at the anonymous block rather than naming the missing
method. A private static `final` implementation or an `EmptyBuildEnvironment`
record would be easier to maintain.
Extract to a private static constant:
```suggestion
return EMPTY_BUILD_ENVIRONMENT;
```
And add above the containing method:
```java
private static final BuildEnvironment EMPTY_BUILD_ENVIRONMENT = new
BuildEnvironment() {
// ... all the existing method implementations ...
};
```
Or better — introduce a `DefaultBuildEnvironment` factory with an `empty()`
method and reuse it here.
--
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]
