gnodet-bot commented on code in PR #13695:
URL: https://github.com/apache/maven/pull/13695#discussion_r4367812127
##########
impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/LookupInvokerLoggingTest.java:
##########
@@ -39,15 +41,13 @@ class LookupInvokerLoggingTest {
@BeforeEach
void setUp() {
- // Save original system property
originalSystemProperty =
System.getProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
- // Clear system property to test configuration file loading
System.clearProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
+ new DefaultLoggerManager();
Review Comment:
⚠️ **Orphaned instance — dead code.** `new DefaultLoggerManager()`
constructs an instance but the result is never stored and never registered via
`LoggerManager.set()`. The constructor does **not** self-register. Tests that
follow call `LoggerManager.get().setRootLoggerLevel()`, which goes to whatever
`Holder.instance` happens to be — likely `NopLoggerManager` from a
ServiceLoader fallback or a previous test's leftover. This construction is a
no-op.
```suggestion
LoggerManager.set(new DefaultLoggerManager());
```
##########
api/maven-api-log/src/main/java/org/apache/maven/api/logging/LoggerManager.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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.api.logging;
+
+import java.util.ServiceLoader;
+
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.Nullable;
+
+/**
+ * Central entry point for Maven's logging system.
+ * <p>
+ * Provides access to loggers, the active {@link LogSink}, level configuration,
+ * and severity tracking for {@code --fail-on-severity}.
+ * <p>
+ * The singleton is resolved lazily on the first {@link #get()} call using
+ * {@link ServiceLoader}. If no provider is found, a no-op fallback is
+ * installed. Embedders and tests can override with {@link
#set(LoggerManager)}.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public interface LoggerManager {
+
+ /**
+ * Returns a {@link Logger} instance for the given name.
+ */
+ @Nonnull
+ Logger getLogger(@Nonnull String name);
+
+ /**
+ * Returns a {@link Logger} instance for the given class.
+ */
+ @Nonnull
+ default Logger getLogger(@Nonnull Class<?> clazz) {
+ return getLogger(clazz.getName());
+ }
+
+ /**
+ * Returns the active log sink, or {@code null} if none is installed.
+ */
+ @Nullable
+ LogSink getLogSink();
+
+ /**
+ * Installs or removes the active log sink.
+ */
+ void setLogSink(@Nullable LogSink sink);
+
+ /**
+ * Returns whether logging at {@code level} is enabled for the named
logger.
+ */
+ boolean isLevelEnabled(@Nonnull String loggerName, @Nonnull LogLevel
level);
+
+ /**
+ * Sets the root logger level (typically from {@code -X}, {@code -q} CLI
flags).
+ * The new level takes effect immediately.
+ */
+ void setRootLoggerLevel(@Nonnull LogLevel level);
+
+ /**
+ * Reloads level configuration and resets severity tracking state.
+ */
+ void reconfigure();
+
+ /**
+ * Returns {@code true} if any log event at or above {@code level}
+ * has been emitted since the last {@link #reconfigure()}.
+ * <p>
+ * Used by {@code --fail-on-severity}: the CLI stores the threshold
+ * and checks at build end.
+ */
+ boolean hasReached(@Nonnull LogLevel level);
+
+ /**
+ * Returns the global {@code LoggerManager} instance.
+ * <p>
+ * On the first call, synchronously discovers an implementation via
+ * {@link ServiceLoader}. If none is found, a no-op fallback is installed.
+ * The result is cached — subsequent calls return immediately.
+ *
+ * @return the global manager, never {@code null}
+ */
+ @Nonnull
+ static LoggerManager get() {
+ LoggerManager mgr = Holder.instance;
+ if (mgr != null) {
+ return mgr;
+ }
+ return Holder.discover();
+ }
+
+ /**
+ * Installs the global {@code LoggerManager} instance.
+ * <p>
+ * Typically used by embedders or tests to override the
+ * {@link ServiceLoader}-discovered implementation.
+ *
+ * @param manager the manager to install, or {@code null} to clear
+ */
+ static void set(LoggerManager manager) {
+ Holder.instance = manager;
+ }
+
+ /**
+ * @hidden
+ */
+ final class Holder {
+ private Holder() {}
+
+ static volatile LoggerManager instance;
+
+ private static final ThreadLocal<Boolean> DISCOVERING = new
ThreadLocal<>();
+
+ static synchronized LoggerManager discover() {
+ LoggerManager mgr = instance;
+ if (mgr != null) {
+ return mgr;
+ }
+
+ // Re-entrancy guard: if ServiceLoader loading triggers logging,
+ // return a no-op manager to avoid infinite recursion.
+ if (DISCOVERING.get() == Boolean.TRUE) {
+ mgr = new NopLoggerManager();
+ instance = mgr;
Review Comment:
⚠️ **Re-entrancy guard permanently poisons `Holder.instance` with
`NopLoggerManager`.**
If `ServiceLoader` loading triggers logging (e.g. a static initializer in
`DefaultLoggerManager` logs something), the re-entrancy guard fires and writes
`instance = new NopLoggerManager()`. The outer `discover()` call then continues
and will eventually overwrite `instance` with the real implementation — but
only if it hasn't already returned early. In the normal flow the outer call
wins and `instance` ends up correct. The problem is the window between the
guard firing and the outer call completing: any thread that calls
`LoggerManager.get()` during that window bypasses the `synchronized discover()`
(instance != null), gets `NopLoggerManager`, and silently loses all logging.
The re-entrancy guard should return `Nop` **locally** to break the recursion
without poisoning the shared `instance`:
```suggestion
if (DISCOVERING.get() == Boolean.TRUE) {
return new NopLoggerManager();
}
```
##########
impl/maven-logging/src/test/java/org/apache/maven/logging/impl/DefaultLoggerTest.java:
##########
@@ -0,0 +1,281 @@
+/*
+ * 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.logging.impl;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.logging.LogEvent;
+import org.apache.maven.api.logging.LogLevel;
+import org.apache.maven.api.logging.LogSink;
+import org.apache.maven.api.logging.LoggerManager;
+import org.apache.maven.api.plugin.Log;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link DefaultLogger}: sink-based dispatch, level gating, child
propagation.
+ */
+class DefaultLoggerTest {
+
+ private String originalLevel;
+
+ @BeforeEach
+ void setUp() {
+ originalLevel =
System.getProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
+ LoggerManager.set(new DefaultLoggerManager());
+ }
+
+ @AfterEach
+ void tearDown() {
+ if (originalLevel != null) {
+ System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL,
originalLevel);
+ } else {
+ System.clearProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
+ }
+ }
Review Comment:
💡 **Missing `LoggerManager.set(null)` in `tearDown` — test isolation gap.**
`setUp()` calls `LoggerManager.set(new DefaultLoggerManager())`, installing a
fresh manager with an `AccumulatingLogSink` into the global `Holder.instance`.
`tearDown()` only restores the system property — it never clears
`Holder.instance`. A subsequent test class that calls `LoggerManager.get()`
without its own `set()` gets this test's stale `DefaultLoggerManager` with the
accumulating sink, silently swallowing all its log events.
```suggestion
@AfterEach
void tearDown() {
LoggerManager.set(null);
if (originalLevel != null) {
System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL,
originalLevel);
} else {
System.clearProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
}
}
```
--
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]
