gnodet-bot commented on code in PR #13695: URL: https://github.com/apache/maven/pull/13695#discussion_r4365601431
########## impl/maven-logging-jul/src/main/java/org/apache/maven/logging/jul/MavenJulHandler.java: ########## @@ -0,0 +1,221 @@ +/* + * 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.jul; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.text.MessageFormat; +import java.util.MissingResourceException; +import java.util.ResourceBundle; +import java.util.concurrent.atomic.AtomicLong; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogManager; +import java.util.logging.LogRecord; +import java.util.logging.Logger; + +import org.apache.maven.api.MonotonicClock; +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.logging.impl.DefaultLogEvent; + +/** + * A JUL {@link Handler} that routes {@code java.util.logging} events into + * Maven's structured logging pipeline via {@link LoggerManager}, preserving + * the rich {@link LogRecord} metadata (source class name, source method name, + * thread ID) that the standard {@code SLF4JBridgeHandler} silently drops. + * <p> + * This handler has no SLF4J dependency — events go directly through + * {@link LoggerManager#get()} for level checking and sink dispatch. + * + * @since 4.1.0 + * @see #install() + */ +public class MavenJulHandler extends Handler { + + private static final AtomicLong SEQUENCE = new AtomicLong(); + + /** + * Re-entrancy guard: set to {@code true} while {@link #publish} is routing + * a JUL event through the sink on this thread. Prevents recursive JUL events + * from re-entering {@code publish} and causing infinite loops. + */ + private static final ThreadLocal<Boolean> IN_PUBLISH = new ThreadLocal<>(); + + static void setInPublishForTest(boolean inPublish) { + if (inPublish) { + IN_PUBLISH.set(Boolean.TRUE); + } else { + IN_PUBLISH.remove(); + } + } + + /** + * Installs this handler on the JUL root logger, removing any + * previously installed handlers. + */ + public static void install() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + for (Handler handler : rootLogger.getHandlers()) { + rootLogger.removeHandler(handler); + } + rootLogger.addHandler(new MavenJulHandler()); + } + + /** + * Returns {@code true} if a {@code MavenJulHandler} is installed + * on the JUL root logger. + */ + public static boolean isInstalled() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + for (Handler handler : rootLogger.getHandlers()) { + if (handler instanceof MavenJulHandler) { + return true; + } + } + return false; + } + + @Override + @SuppressWarnings("deprecation") // LogRecord.getThreadID() + public void publish(LogRecord record) { + if (record == null || !isLoggable(record)) { + return; + } + + if (IN_PUBLISH.get() != null) { + return; + } + + LoggerManager mgr = LoggerManager.get(); + if (mgr == null) { + return; + } + + String loggerName = record.getLoggerName(); + if (loggerName == null) { + loggerName = ""; + } + + LogLevel level = julLevelToLogLevel(record.getLevel()); + if (!mgr.isLevelEnabled(loggerName, level)) { + return; + } + + IN_PUBLISH.set(Boolean.TRUE); + try { + LogSink sink = mgr.getLogSink(); + if (sink == null) { + return; + } + + String message = formatMessage(record); + Throwable throwable = record.getThrown(); + + String sourceClass = null; + String sourceMethod = null; + long threadId = -1; + String threadName = null; + + if (sink.needsMetadata()) { + sourceClass = record.getSourceClassName(); + sourceMethod = record.getSourceMethodName(); + threadId = record.getLongThreadID(); + threadName = Thread.currentThread().getName(); + } + + String stackTrace = throwable != null ? formatStackTrace(throwable) : null; + + LogEvent event = new DefaultLogEvent( + MonotonicClock.now(), + level, + message, + loggerName, + stackTrace, + throwable, + sourceClass, + sourceMethod, + threadId, + SEQUENCE.getAndIncrement(), + threadName, + null, + null); + + sink.accept(event); Review Comment: 🔴 **`--fail-on-severity` is still broken for JUL-routed events.** `MavenJulHandler.publish()` bypasses `DefaultLogger` entirely — it calls `sink.accept(event)` but never calls `mgr.record(level)`. The fix added to `DefaultLogger.doLog()` only covers the fluent `Logger` API and SLF4J bridge paths. Any JUL event (e.g. from a third-party library using `java.util.logging`) will not trigger a build failure even when it exceeds the configured `--fail-on-severity` threshold. ```suggestion sink.accept(event); if (mgr instanceof DefaultLoggerManager dlm) { dlm.record(level); } ``` ########## impl/maven-logging/src/main/java/org/apache/maven/logging/impl/DefaultLoggerManager.java: ########## @@ -0,0 +1,305 @@ +/* + * 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.io.InputStream; +import java.util.List; +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; + +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.Logger; +import org.apache.maven.api.logging.LoggerManager; + +/** + * Default {@link LoggerManager} implementation. Owns: + * <ul> + * <li>Per-logger level configuration (loaded from properties files)</li> + * <li>The active {@link LogSink}</li> + * <li>Severity tracking for {@code --fail-on-severity}</li> + * </ul> + * <p> + * This class has no SLF4J dependency. The SLF4J adapter layer creates + * an instance and registers it via {@link LoggerManager#set(LoggerManager)}. + */ +public class DefaultLoggerManager implements LoggerManager { + + private static final String CONFIGURATION_FILE = "maven.logger.properties"; + + @Deprecated(since = "4.0.0") + private static final String LEGACY_CONFIGURATION_FILE = "simplelogger.properties"; + + private static final String LEGACY_PREFIX = "org.slf4j.simpleLogger."; + + public static final int LOG_LEVEL_TRACE = 0; + public static final int LOG_LEVEL_DEBUG = 10; + public static final int LOG_LEVEL_INFO = 20; + public static final int LOG_LEVEL_WARN = 30; + public static final int LOG_LEVEL_ERROR = 40; + public static final int LOG_LEVEL_OFF = 50; + + private volatile LogSink logSink; + private volatile int defaultLogLevel = LOG_LEVEL_INFO; + private final AtomicReference<LogLevel> maxReached = new AtomicReference<>(null); + private final Properties properties = new Properties(); + private final ConcurrentHashMap<String, Integer> levelCache = new ConcurrentHashMap<>(); + + public DefaultLoggerManager() { + this.logSink = new AccumulatingLogSink(); + LoggerManager.set(this); + loadConfiguration(); + } + + // -- LogManager: loggers -- + + @Override + public Logger getLogger(String name) { + return new DefaultLogger(name); + } + + // -- LogManager: sink -- + + @Override + public LogSink getLogSink() { + return logSink; + } + + @Override + public void setLogSink(LogSink sink) { + LogSink old = this.logSink; + // Drain BEFORE making the new sink visible to concurrent doLog() callers. + // If we swapped first, a concurrent logger could write directly to the new sink + // while drainTo() is still replaying older buffered events — breaking ordering. + if (old instanceof AccumulatingLogSink acc && sink != null) { + acc.drainTo(sink); + } + this.logSink = sink; + } + + /** + * A sink that buffers events until a real sink is installed. + * Used as the default during early bootstrap. + */ + private static class AccumulatingLogSink implements LogSink { + private final List<LogEvent> events = new CopyOnWriteArrayList<>(); + + @Override + public void accept(LogEvent event) { + events.add(event); + } + + void drainTo(LogSink target) { + for (LogEvent event : events) { + target.accept(event); + } + events.clear(); Review Comment: 🟡 **`drainTo()` has a TOCTOU race that can silently drop bootstrap events.** `CopyOnWriteArrayList`'s for-each iterator captures a snapshot at the moment of iteration. Any event added by a concurrent `doLog()` thread between the end of the `for` loop and the `events.clear()` call is never forwarded to the new sink — it is not in the snapshot, and `clear()` wipes it. During bootstrap (before the real sink is installed), multiple threads may be logging concurrently, making this a real window for silent event loss. Replace `CopyOnWriteArrayList` with `ConcurrentLinkedQueue` and use `poll()` to drain atomically: ```suggestion private static class AccumulatingLogSink implements LogSink { private final java.util.concurrent.ConcurrentLinkedQueue<LogEvent> events = new java.util.concurrent.ConcurrentLinkedQueue<>(); @Override public void accept(LogEvent event) { events.add(event); } void drainTo(LogSink target) { LogEvent event; while ((event = events.poll()) != null) { target.accept(event); } } } ``` -- 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]
