diff --git a/log4j-to-slf4j/pom.xml b/log4j-to-slf4j/pom.xml index 3a6b0d9e850..a2d6f0a6647 100644 --- a/log4j-to-slf4j/pom.xml +++ b/log4j-to-slf4j/pom.xml @@ -153,6 +153,45 @@ + + + + + + + + java8-incompat-fixes + + + + + !env.CI + + + + + + + + org.apache.maven.plugins + maven-surefire-plugin + + --add-opens java.base/java.lang=ALL-UNNAMED + + + + + + + + diff --git a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java index 26e94c67b35..ff9410f33e1 100644 --- a/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java +++ b/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/SLF4JLogger.java @@ -42,17 +42,21 @@ public class SLF4JLogger extends AbstractLogger { private final org.slf4j.Logger logger; private final LocationAwareLogger locationAwareLogger; + private final boolean useThreadLocal; public SLF4JLogger(final String name, final MessageFactory messageFactory, final org.slf4j.Logger logger) { - super(name, messageFactory); - this.logger = logger; - this.locationAwareLogger = logger instanceof LocationAwareLogger ? (LocationAwareLogger) logger : null; + this(name, messageFactory, logger, Constants.ENABLE_THREADLOCALS); } public SLF4JLogger(final String name, final org.slf4j.Logger logger) { - super(name); + this(name, null, logger); + } + + SLF4JLogger(String name, MessageFactory messageFactory, org.slf4j.Logger logger, boolean useThreadLocal) { + super(name, messageFactory); this.logger = logger; this.locationAwareLogger = logger instanceof LocationAwareLogger ? (LocationAwareLogger) logger : null; + this.useThreadLocal = useThreadLocal; } private int convertLevel(final Level level) { @@ -364,8 +368,8 @@ public LogBuilder atFatal() { @Override protected LogBuilder getLogBuilder(final Level level) { - final SLF4JLogBuilder builder = logBuilder.get(); - return Constants.ENABLE_THREADLOCALS && !builder.isInUse() + SLF4JLogBuilder builder; + return useThreadLocal && !(builder = logBuilder.get()).isInUse() ? builder.reset(this, level) : new SLF4JLogBuilder(this, level); } diff --git a/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java b/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java similarity index 82% rename from log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java rename to log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java index 6c378ad9583..9e3a5856a06 100644 --- a/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/LoggerTest.java +++ b/log4j-to-slf4j/src/test/java/org/apache/logging/slf4j/SLF4JLoggerTest.java @@ -32,10 +32,13 @@ import ch.qos.logback.classic.LoggerContext; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.testUtil.StringListAppender; +import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.util.Date; import java.util.List; +import org.apache.logging.log4j.LogBuilder; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.ThreadContext; @@ -45,13 +48,17 @@ import org.apache.logging.log4j.spi.AbstractLogger; import org.apache.logging.log4j.spi.MessageFactory2Adapter; import org.apache.logging.log4j.test.junit.UsingStatusListener; +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.junitpioneer.jupiter.Issue; import org.slf4j.MDC; @UsingStatusListener @LoggerContextSource -class LoggerTest { +class SLF4JLoggerTest { private static final Object OBJ = new Object(); // Log4j objects @@ -265,4 +272,38 @@ void mdcCannotContainNullKey() { // expected } } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + @Issue("https://github.com/apache/logging-log4j2/issues/3819") + void threadLocalUsage(boolean useThreadLocal) throws ReflectiveOperationException { + // Reset the static ThreadLocal in SLF4JLogger + getLogBuilderThreadLocal().remove(); + final org.slf4j.Logger slf4jLogger = context.getLogger(getClass()); + Logger logger = new SLF4JLogger(slf4jLogger.getName(), null, slf4jLogger, useThreadLocal); + LogBuilder builder1 = logger.atInfo(); + builder1.log("Test message"); + LogBuilder builder2 = logger.atInfo(); + builder2.log("Another test message"); + // Check if the same builder is reused based on the useThreadLocal flag + Assertions.assertThat(isThreadLocalPresent()) + .as("ThreadLocal should be present iff useThreadLocal is enabled") + .isEqualTo(useThreadLocal); + Assertions.assertThat(builder2 == builder1) + .as("Builder2 should be the same as Builder1 iff useThreadLocal is enabled") + .isEqualTo(useThreadLocal); + } + + private static boolean isThreadLocalPresent() throws ReflectiveOperationException { + Method isPresentMethod = ThreadLocal.class.getDeclaredMethod("isPresent"); + isPresentMethod.setAccessible(true); + ThreadLocal threadLocal = getLogBuilderThreadLocal(); + return (boolean) isPresentMethod.invoke(threadLocal); + } + + private static ThreadLocal getLogBuilderThreadLocal() throws ReflectiveOperationException { + Field logBuilderField = SLF4JLogger.class.getDeclaredField("logBuilder"); + logBuilderField.setAccessible(true); + return (ThreadLocal) logBuilderField.get(null); + } } diff --git a/log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/LoggerTest.xml b/log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/SLF4JLoggerTest.xml similarity index 100% rename from log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/LoggerTest.xml rename to log4j-to-slf4j/src/test/resources/org/apache/logging/slf4j/SLF4JLoggerTest.xml diff --git a/src/changelog/.2.x.x/3819_logback-builder-reuse.xml b/src/changelog/.2.x.x/3819_logback-builder-reuse.xml new file mode 100644 index 00000000000..ea67aa50d04 --- /dev/null +++ b/src/changelog/.2.x.x/3819_logback-builder-reuse.xml @@ -0,0 +1,12 @@ + + + + + Fix potential memory leak involving `LogBuilder` in Log4j API to Logback bridge. + +