gnodet-bot commented on code in PR #13695:
URL: https://github.com/apache/maven/pull/13695#discussion_r4367292975
##########
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:
⚠️ **[Still open] Orphaned instance — dead code.** `new
DefaultLoggerManager()` constructs an instance that is immediately discarded.
It is never passed to `LoggerManager.set(…)`, so it has no effect on the
`Holder.instance` field used by subsequent `LoggerManager.get()` calls.
```suggestion
LoggerManager.set(new DefaultLoggerManager());
```
Without this fix the tests interact with whatever `Holder.instance` was left
by previous test classes (the `DefaultLoggerManager` installed by
`DefaultLoggerTest` or a `NopLoggerManager` poisoned by the re-entrancy guard).
This also means the `tearDown` should clear the instance to avoid leaking it:
```java
@AfterEach
void tearDown() {
LoggerManager.set(null);
if (originalSystemProperty != null) {
System.setProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL,
originalSystemProperty);
} else {
System.clearProperty(Constants.MAVEN_LOGGER_DEFAULT_LOG_LEVEL);
}
}
```
##########
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;
+ return mgr;
+ }
+
Review Comment:
⚠️ **[Still open] Re-entrancy guard permanently poisons `Holder.instance`
with `NopLoggerManager`.** `discover()` is `synchronized`, but `get()` (line
104) reads `Holder.instance` without acquiring the monitor. When the
re-entrancy path is taken:
1. Thread A enters `discover()` and sets `instance = new NopLoggerManager()`
at line 143 (inside the `synchronized` block).
2. Before the `synchronized` block returns, thread B calls `get()` — it sees
a non-null `instance` (the `NopLoggerManager`) and returns it **permanently**.
Thread B will never call `discover()` again.
3. Thread A completes, but thread B already cached the `NopLoggerManager`.
The fix is to return a fresh `NopLoggerManager` locally without touching the
shared field:
```suggestion
if (DISCOVERING.get() == Boolean.TRUE) {
return new NopLoggerManager();
}
```
The `instance` field must only be written once — to the real implementation
(or a user-installed override via `set()`). Writing a temporary sentinel to the
shared field is incompatible with the unsynchronized fast-path read in `get()`.
##########
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:
💡 **[Still open] Missing `LoggerManager.set(null)` in `tearDown` — test
isolation gap.** `setUp` correctly calls `LoggerManager.set(new
DefaultLoggerManager())` to install a fresh instance before each test. However,
`tearDown` only restores the system property and does not clear
`Holder.instance`. Any test class that runs after this one and relies on
`Holder.instance` being `null` (to trigger ServiceLoader discovery) will
instead get this leaked `DefaultLoggerManager`.
```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]
