gnodet-bot commented on code in PR #13539:
URL: https://github.com/apache/maven/pull/13539#discussion_r4293375769
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/EventSpyImpl.java:
##########
@@ -52,22 +62,47 @@ public void onEvent(Object arg) throws Exception {
EventType eventType = convert(ee.getType());
Collection<Listener> listeners = session.getListeners();
if (!listeners.isEmpty()) {
- org.apache.maven.api.ExecutionEvent event = new
DefaultEvent(session, ee, eventType);
- for (Listener listener : listeners) {
- try {
- listener.onEvent(event);
- } catch (RuntimeException e) {
- LOGGER.warn(
- "Failed to notify listener {} about {}",
- listener.getClass().getName(),
- eventType,
- e);
- }
+ notify(listeners, new DefaultEvent(session, ee, eventType),
eventType);
+ }
+ } else if (arg instanceof DependencyResolutionRequest request) {
+ pendingResolution.set(request);
Review Comment:
💡 **Defensive suggestion:** `pendingResolution` is set here but only cleaned
up when a `DependencyResolutionResult` event arrives. If
`dependenciesResolver.resolve()` throws an unexpected `RuntimeException` or
`Error` (not `DependencyResolutionException`), `LifecycleDependencyResolver`
will propagate the exception without dispatching a result event, leaving the
ThreadLocal set for the lifetime of the thread.
In a thread-pool build this is a minor resource leak per failed resolution;
in a thread-reuse scenario it could cause a stale request to be matched to the
next resolution's result. A `try/finally` around the `resolve()` call in
`LifecycleDependencyResolver` would be the cleanest fix, but alternatively the
`onEvent(DependencyResolutionResult)` branch could tolerate a missing request
by also calling `pendingResolution.remove()` unconditionally (it already does
this). The risk is low in practice since the exception would also abort the
build, but it's worth a TODO comment at minimum.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultDependencyResolutionEvent.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.internal.impl;
+
+import java.util.List;
+import java.util.Objects;
+import java.util.Optional;
+
+import org.apache.maven.api.Dependency;
+import org.apache.maven.api.DependencyResolutionEvent;
+import org.apache.maven.api.DependencyResolutionEventType;
+import org.apache.maven.api.Project;
+import org.apache.maven.api.Session;
+import org.apache.maven.project.DependencyResolutionResult;
+
+/**
+ * Maven 4 view of a dependency resolution, created from the legacy request
and result.
+ */
+class DefaultDependencyResolutionEvent implements DependencyResolutionEvent {
+ private final InternalMavenSession session;
+ private final Project project;
+ private final DependencyResolutionEventType type;
+ private final List<Dependency> resolved;
+ private final List<Dependency> unresolved;
+ private final Exception exception;
+
+ /** Creates the event fired before the dependencies of the project are
resolved. */
+ DefaultDependencyResolutionEvent(InternalMavenSession session, Project
project) {
+ this.session = Objects.requireNonNull(session, "session");
+ this.project = Objects.requireNonNull(project, "project");
+ this.type = DependencyResolutionEventType.STARTED;
+ this.resolved = List.of();
+ this.unresolved = List.of();
+ this.exception = null;
+ }
+
+ /** Creates the event fired once the dependencies of the project have been
resolved or have failed to. */
+ DefaultDependencyResolutionEvent(InternalMavenSession session, Project
project, DependencyResolutionResult result) {
+ this.session = Objects.requireNonNull(session, "session");
+ this.project = Objects.requireNonNull(project, "project");
+ this.resolved = result.getResolvedDependencies().stream()
+ .map(session::getDependency)
+ .toList();
+ this.unresolved = result.getUnresolvedDependencies().stream()
+ .map(session::getDependency)
+ .toList();
+ Exception failure = null;
+ if (!result.getCollectionErrors().isEmpty()) {
+ failure = result.getCollectionErrors().get(0);
+ } else {
+ for (org.eclipse.aether.graph.Dependency dependency :
result.getUnresolvedDependencies()) {
+ List<Exception> errors =
result.getResolutionErrors(dependency);
+ if (errors != null && !errors.isEmpty()) {
+ failure = errors.get(0);
+ break;
+ }
+ }
+ }
+ this.exception = failure;
+ this.type = failure != null || !unresolved.isEmpty()
+ ? DependencyResolutionEventType.FAILED
+ : DependencyResolutionEventType.SUCCEEDED;
Review Comment:
🔍 **API contract observation:** When `unresolved` is non-empty but every
unresolved dependency has empty `getResolutionErrors()` (and there are no
collection errors), `failure` remains `null` yet `type` is set to `FAILED`.
Callers of `DependencyResolutionEvent` who follow the documented contract —
checking `type() == FAILED` then calling `exception()` — will get
`Optional.empty()` in this edge case, which may be surprising.
This scenario is unlikely in practice (a dependency ends up unresolved with
no errors), but a brief Javadoc note on `exception()` clarifying that it may be
empty even when `type()` is `FAILED` would prevent confusion for API consumers:
```suggestion
/**
* {@return the primary failure of the resolution, if any}
* This is the first dependency collection error or, when there is none,
the first resolution error.
* May be empty even when {@link #type()} is {@link
DependencyResolutionEventType#FAILED}
* if dependencies failed to resolve without a reported error.
*/
```
--
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]
