gnodet-bot commented on code in PR #13539:
URL: https://github.com/apache/maven/pull/13539#discussion_r4293517187
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/EventSpyImpl.java:
##########
@@ -52,18 +67,51 @@ 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);
- }
- }
+ dispatch(listeners, new DefaultEvent(session, ee, eventType),
eventType);
+ }
+ } else if (arg instanceof DependencyResolutionRequest request) {
+ pendingResolution.set(new WeakReference<>(request));
Review Comment:
💡 **Minor — stale `ThreadLocal` entry on unexpected `RuntimeException`**
If `LifecycleDependencyResolver.getDependencies()` throws an unexpected
`RuntimeException` (not caught by `catch (DependencyResolutionException)`), no
`DependencyResolutionResult` event is dispatched and
`pendingResolution.remove()` is never called. The `WeakReference` mitigates the
referent leak, but the `ThreadLocal` slot itself remains set for the life of
the thread (e.g. in a thread-pool scenario).
A simple defensive fix: call `remove()` before `set()` so that any stale
entry from a prior failed resolution is cleared when a new request begins:
```suggestion
pendingResolution.remove();
pendingResolution.set(new WeakReference<>(request));
```
This is a best-effort cleanup — the `WeakReference` already prevents a hard
memory leak, so this is purely defensive.
--
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]
