gnodet-bot commented on code in PR #12694: URL: https://github.com/apache/maven/pull/12694#discussion_r4057777122
########## impl/maven-logging/src/main/java/org/apache/maven/slf4j/MavenJulHandler.java: ########## @@ -0,0 +1,296 @@ +/* + * 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.slf4j; + +import java.text.MessageFormat; +import java.util.MissingResourceException; +import java.util.ResourceBundle; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; +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.slf4j.LoggerFactory; +import org.slf4j.spi.LocationAwareLogger; + +/** + * A JUL {@link Handler} that routes {@code java.util.logging} events into + * Maven's structured logging pipeline, preserving the rich {@link LogRecord} + * metadata that the standard {@code SLF4JBridgeHandler} silently drops + * (source class name, source method name, thread ID). + * <p> + * All JUL events are routed through SLF4J so that {@link MavenSimpleLogger} + * produces a consistent {@code formattedMessage} (with timestamp, logger name, + * and ANSI styling) regardless of the event's origin. The JUL metadata is + * stashed in a thread-local <em>before</em> the SLF4J call so that downstream + * consumers (e.g. {@code ProjectBuildLogAppender}) can read it when + * constructing a structured {@code LogEvent}. + * <p> + * Usage — replace the standard SLF4J bridge in {@code LookupInvoker}: + * <pre> + * MavenJulHandler.install(); + * </pre> + * + * @since 4.1.0 + * @see #install() + * @see #getJulMetadata() + */ +public class MavenJulHandler extends Handler { + + /** + * JUL metadata captured from a {@link LogRecord} that would otherwise + * be lost when bridging to SLF4J. + * + * @param sourceClassName the source class, or {@code null} + * @param sourceMethodName the source method, or {@code null} + * @param threadId the originating thread ID + */ + public record JulMetadata(String sourceClassName, String sourceMethodName, long threadId) {} + + private static final ThreadLocal<JulMetadata> METADATA = new ThreadLocal<>(); + + /** + * Private SLF4J logger cache using {@link ConcurrentMap#putIfAbsent} + * instead of {@link ConcurrentMap#computeIfAbsent}. This avoids the + * {@code ConcurrentHashMap.computeIfAbsent} reentrancy bug + * ({@code IllegalStateException("Recursive update")}) that occurs + * when a JUL event fires during SLF4J logger initialization: the + * handler's {@code publish()} calls {@code LoggerFactory.getLogger()}, + * which internally uses {@code computeIfAbsent}, and if that triggers + * another JUL event whose logger name hashes to the same bucket, + * {@code ConcurrentHashMap} throws. {@code putIfAbsent} is safe + * against reentrancy — worst case, two threads create the same + * logger and one is discarded. + */ + private static final ConcurrentMap<String, org.slf4j.Logger> LOGGER_CACHE = new ConcurrentHashMap<>(); + + /** + * Re-entrancy guard: set to {@code true} while {@link #publish} is routing + * a JUL event through SLF4J on this thread. Prevents recursive JUL events + * (e.g. JLine's {@code StyleResolver} calling {@code java.util.logging.Logger} + * while inside {@link MavenSimpleLogger#renderLevel} lazy-initialisation, + * which in turn is triggered by a JUL event during terminal construction) + * from re-entering {@code publish} and crashing with + * {@code ConcurrentHashMap.computeIfAbsent IllegalStateException("Recursive update")}. + */ + private static final ThreadLocal<Boolean> IN_PUBLISH = new ThreadLocal<>(); + + /** + * Returns the JUL metadata for the current log event being processed, + * or {@code null} if the current log event did not originate from JUL. + * <p> + * This method is intended to be called from within a + * {@link MavenSimpleLogger.LogSink} callback (e.g. in + * {@code ProjectBuildLogAppender.accept()}). + * + * @return the current JUL metadata, or {@code null} + */ + public static JulMetadata getJulMetadata() { + return METADATA.get(); + } + + /** + * Installs this handler on the JUL root logger, removing any + * previously installed handlers. This replaces the standard + * {@code SLF4JBridgeHandler.install()} call. + */ + public static void install() { + Logger rootLogger = LogManager.getLogManager().getLogger(""); + // Remove all existing handlers (including any SLF4JBridgeHandler) + for (Handler handler : rootLogger.getHandlers()) { + rootLogger.removeHandler(handler); + } + rootLogger.addHandler(new MavenJulHandler()); + // Note: we intentionally do NOT set rootLogger.setLevel(Level.ALL) + // here. Setting it eagerly floods JUL events during SLF4J bootstrap, + // triggering ConcurrentHashMap.computeIfAbsent reentrancy in the + // SLF4J logger factory ("Recursive update"). The JUL root default + // (INFO) is fine — callers that need FINE/FINEST events (e.g. -X + // debug mode) should set the JUL root level after SLF4J is fully + // initialized. + } + + /** + * 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 + public void publish(LogRecord record) { Review Comment: **[low] `publish()` skips `isLoggable(record)` — violates `Handler` contract** `java.util.logging.Handler` declares `publish()` implementations responsible for checking `isLoggable(record)` before processing. The standard `SLF4JBridgeHandler` calls it early: ```java public void publish(LogRecord record) { if (!isLoggable(record)) { return; } ... } ``` `MavenJulHandler` skips this call entirely, meaning any `Filter` registered on this handler via `setFilter()` is silently ignored. For Maven's own deployment this has zero practical impact (no filter is ever set), but embedders that configure JUL handlers with custom `Filter` objects will find their filters bypassed. Suggested fix — add the check right after the null guard: ```suggestion public void publish(LogRecord record) { if (record == null) { return; } if (!isLoggable(record)) { return; } ``` -- 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]
