You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by ck...@apache.org on 2022/02/18 17:11:28 UTC
[logging-log4j2] branch release-2.x updated: [LOG4J2-3404] Create default layouts using the available Configuration (#756)
This is an automated email from the ASF dual-hosted git repository.
ckozak pushed a commit to branch release-2.x
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/release-2.x by this push:
new 9ffa643 [LOG4J2-3404] Create default layouts using the available Configuration (#756)
9ffa643 is described below
commit 9ffa643d69cdca4764bc2c4c0fe529379d17db2b
Author: ppkarwasz <pi...@karwasz.org>
AuthorDate: Fri Feb 18 18:11:18 2022 +0100
[LOG4J2-3404] Create default layouts using the available Configuration (#756)
This PR provides a `Configuration` argument whenever possible.
`PatternLayout` no longer creates (and leaks) DefaultConfiguration instances
---
.../log4j/core/appender/AbstractAppender.java | 4 +--
.../log4j/core/appender/AbstractManager.java | 7 ++++
.../log4j/core/appender/OutputStreamAppender.java | 6 ++--
.../log4j/core/appender/WriterAppender.java | 12 +++++--
.../log4j/core/config/AbstractConfiguration.java | 12 ++++---
.../logging/log4j/core/layout/AbstractLayout.java | 2 +-
.../log4j/core/layout/AbstractStringLayout.java | 20 +++++++++---
.../logging/log4j/core/layout/PatternLayout.java | 8 ++---
.../logging/log4j/core/pattern/PatternParser.java | 10 +++---
.../core/appender/AbstractAppenderBuilderTest.java | 37 ++++++++++++++++++++++
.../core/layout/AbstractStringLayoutTest.java | 26 +++++++++++++--
.../log4j/web/appender/ServletAppender.java | 8 ++---
.../log4j/web/appender/ServletAppender.java | 8 ++---
13 files changed, 117 insertions(+), 43 deletions(-)
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractAppender.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractAppender.java
index 2ffbf15..fd7bba3 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractAppender.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractAppender.java
@@ -76,14 +76,14 @@ public abstract class AbstractAppender extends AbstractFilterable implements App
public Layout<? extends Serializable> getOrCreateLayout() {
if (layout == null) {
- return PatternLayout.createDefaultLayout();
+ return PatternLayout.createDefaultLayout(configuration);
}
return layout;
}
public Layout<? extends Serializable> getOrCreateLayout(final Charset charset) {
if (layout == null) {
- return PatternLayout.newBuilder().withCharset(charset).build();
+ return PatternLayout.newBuilder().withCharset(charset).withConfiguration(configuration).build();
}
return layout;
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractManager.java
index 72a8b4a..86bd680 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractManager.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractManager.java
@@ -205,6 +205,13 @@ public abstract class AbstractManager implements AutoCloseable {
}
/**
+ * For testing purposes.
+ */
+ static int getManagerCount() {
+ return MAP.size();
+ }
+
+ /**
* May be overridden by managers to perform processing while the manager is being released and the
* lock is held. A timeout is passed for implementors to use as they see fit.
* @param timeout timeout
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java
index dc6f352..8b1fae6 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/OutputStreamAppender.java
@@ -57,10 +57,8 @@ public final class OutputStreamAppender extends AbstractOutputStreamAppender<Out
@Override
public OutputStreamAppender build() {
- final Layout<? extends Serializable> layout = getLayout();
- final Layout<? extends Serializable> actualLayout = layout == null ? PatternLayout.createDefaultLayout()
- : layout;
- return new OutputStreamAppender(getName(), actualLayout, getFilter(), getManager(target, follow, actualLayout),
+ final Layout<? extends Serializable> layout = getOrCreateLayout();
+ return new OutputStreamAppender(getName(), layout, getFilter(), getManager(target, follow, layout),
ignoreExceptions, getPropertyArray());
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterAppender.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterAppender.java
index 408fed1..6921d56 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterAppender.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/WriterAppender.java
@@ -16,11 +16,13 @@
*/
package org.apache.logging.log4j.core.appender;
+import java.io.Serializable;
import java.io.Writer;
import org.apache.logging.log4j.core.Appender;
import org.apache.logging.log4j.core.Core;
import org.apache.logging.log4j.core.Filter;
+import org.apache.logging.log4j.core.Layout;
import org.apache.logging.log4j.core.StringLayout;
import org.apache.logging.log4j.core.config.Property;
import org.apache.logging.log4j.core.config.plugins.Plugin;
@@ -47,9 +49,13 @@ public final class WriterAppender extends AbstractWriterAppender<WriterManager>
@Override
public WriterAppender build() {
- final StringLayout layout = (StringLayout) getLayout();
- final StringLayout actualLayout = layout != null ? layout : PatternLayout.createDefaultLayout();
- return new WriterAppender(getName(), actualLayout, getFilter(), getManager(target, follow, actualLayout),
+ final Layout<? extends Serializable> layout = getOrCreateLayout();
+ if (!(layout instanceof StringLayout)) {
+ LOGGER.error("Layout must be a StringLayout to log to ServletContext");
+ return null;
+ }
+ final StringLayout stringLayout = (StringLayout) layout;
+ return new WriterAppender(getName(), stringLayout, getFilter(), getManager(target, follow, stringLayout),
isIgnoreExceptions(), getPropertyArray());
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java
index 8c2755b..d6da277 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java
@@ -714,6 +714,12 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement
setParents();
}
+ public static Level getDefaultLevel() {
+ final String levelName = PropertiesUtil.getProperties().getStringProperty(DefaultConfiguration.DEFAULT_LEVEL,
+ Level.ERROR.name());
+ return Level.valueOf(levelName);
+ }
+
protected void setToDefault() {
// LOG4J2-1176 facilitate memory leak investigation
setName(DefaultConfiguration.DEFAULT_NAME + "@" + Integer.toHexString(hashCode()));
@@ -727,11 +733,7 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement
final LoggerConfig rootLoggerConfig = getRootLogger();
rootLoggerConfig.addAppender(appender, null, null);
- final Level defaultLevel = Level.ERROR;
- final String levelName = PropertiesUtil.getProperties().getStringProperty(DefaultConfiguration.DEFAULT_LEVEL,
- defaultLevel.name());
- final Level level = Level.valueOf(levelName);
- rootLoggerConfig.setLevel(level != null ? level : defaultLevel);
+ rootLoggerConfig.setLevel(getDefaultLevel());
}
/**
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractLayout.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractLayout.java
index 6a7f175..7496a44 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractLayout.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractLayout.java
@@ -129,7 +129,7 @@ public abstract class AbstractLayout<T extends Serializable> implements Layout<T
* Constructs a layout with an optional header and footer.
*
* @param configuration
- * The configuration
+ * The configuration. May be null.
* @param header
* The header to include when the stream is opened. May be null.
* @param footer
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
index 8997d41..3461507 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/AbstractStringLayout.java
@@ -19,8 +19,11 @@ package org.apache.logging.log4j.core.layout;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.StringLayout;
+import org.apache.logging.log4j.core.config.AbstractConfiguration;
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.LoggerConfig;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
@@ -176,7 +179,7 @@ public abstract class AbstractStringLayout extends AbstractLayout<String> implem
/**
* Builds a new layout.
- * @param config the configuration
+ * @param config the configuration. May be null.
* @param aCharset the charset used to encode the header bytes, footer bytes and anything else that needs to be
* converted from strings to bytes.
* @param headerSerializer the header bytes serializer
@@ -264,10 +267,19 @@ public abstract class AbstractStringLayout extends AbstractLayout<String> implem
if (serializer == null) {
return null;
}
- final LoggerConfig rootLogger = getConfiguration().getRootLogger();
+ final String loggerName;
+ final Level level;
+ if (configuration != null) {
+ final LoggerConfig rootLogger = configuration.getRootLogger();
+ loggerName = rootLogger.getName();
+ level = rootLogger.getLevel();
+ } else {
+ loggerName = LogManager.ROOT_LOGGER_NAME;
+ level = AbstractConfiguration.getDefaultLevel();
+ }
// Using "" for the FQCN, does it matter?
- final LogEvent logEvent = getLogEventFactory().createEvent(rootLogger.getName(), null, Strings.EMPTY,
- rootLogger.getLevel(), null, null, null);
+ final LogEvent logEvent = getLogEventFactory().createEvent(loggerName, null, Strings.EMPTY,
+ level, null, null, null);
return serializer.toSerializable(logEvent);
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/PatternLayout.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/PatternLayout.java
index 6ba5882..2f754c6 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/PatternLayout.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/PatternLayout.java
@@ -25,7 +25,6 @@ import java.util.Map;
import org.apache.logging.log4j.core.Layout;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.logging.log4j.core.config.Configuration;
-import org.apache.logging.log4j.core.config.DefaultConfiguration;
import org.apache.logging.log4j.core.config.Node;
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginAttribute;
@@ -243,7 +242,7 @@ public final class PatternLayout extends AbstractStringLayout {
/**
* Creates a PatternParser.
- * @param config The Configuration.
+ * @param config The Configuration or {@code null}.
* @return The PatternParser.
*/
public static PatternParser createPatternParser(final Configuration config) {
@@ -763,10 +762,7 @@ public final class PatternLayout extends AbstractStringLayout {
@Override
public PatternLayout build() {
- // fall back to DefaultConfiguration
- if (configuration == null) {
- configuration = new DefaultConfiguration();
- }
+ // should work with a null configuration
return new PatternLayout(configuration, regexReplacement, pattern, patternSelector, charset,
alwaysWriteExceptions, disableAnsi, noConsoleNoAnsi, header, footer);
}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java
index dc77f87..8ac6563 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/PatternParser.java
@@ -103,11 +103,11 @@ public final class PatternParser {
* Constructor.
*
* @param config
- * The current Configuration.
+ * The current Configuration or {@code null}.
* @param converterKey
* The key to lookup the converters.
* @param expected
- * The expected base Class of each Converter.
+ * The expected base Class of each Converter or {@code null}.
*/
public PatternParser(final Configuration config, final String converterKey, final Class<?> expected) {
this(config, converterKey, expected, null);
@@ -117,13 +117,13 @@ public final class PatternParser {
* Constructor.
*
* @param config
- * The current Configuration.
+ * The current Configuration or {@code null}.
* @param converterKey
* The key to lookup the converters.
* @param expectedClass
- * The expected base Class of each Converter.
+ * The expected base Class of each Converter or {@code null}.
* @param filterClass
- * Filter the returned plugins after calling the plugin manager.
+ * Filter the returned plugins after calling the plugin manager, can be {@code null}.
*/
public PatternParser(final Configuration config, final String converterKey, final Class<?> expectedClass,
final Class<?> filterClass) {
diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/AbstractAppenderBuilderTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/AbstractAppenderBuilderTest.java
new file mode 100644
index 0000000..7481fb5
--- /dev/null
+++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/AbstractAppenderBuilderTest.java
@@ -0,0 +1,37 @@
+/*
+ * 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.logging.log4j.core.appender;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.config.DefaultConfiguration;
+import org.junit.jupiter.api.Test;
+
+public class AbstractAppenderBuilderTest {
+
+ @Test
+ public void testDefaultLayoutLeak() {
+ int expected = AbstractManager.getManagerCount();
+ final Configuration configuration = new DefaultConfiguration();
+ ConsoleAppender appender = ConsoleAppender.newBuilder().setConfiguration(configuration).setName("test").build();
+ configuration.addAppender(appender);
+ configuration.start();
+ configuration.stop();
+ assertEquals(expected, AbstractManager.getManagerCount(), "No managers should be left after shutdown.");
+ }
+}
diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/layout/AbstractStringLayoutTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/layout/AbstractStringLayoutTest.java
index b29b25d..0a7edf5 100644
--- a/log4j-core/src/test/java/org/apache/logging/log4j/core/layout/AbstractStringLayoutTest.java
+++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/layout/AbstractStringLayoutTest.java
@@ -15,23 +15,33 @@ package org.apache.logging.log4j.core.layout;/*
* limitations under the license.
*/
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
import java.nio.charset.Charset;
import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.config.DefaultConfiguration;
+import org.apache.logging.log4j.core.layout.AbstractStringLayout.Serializer;
+import org.apache.logging.log4j.core.layout.PatternLayout.SerializerBuilder;
import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.*;
-
/**
* Tests AbstractStringLayout.
*/
public class AbstractStringLayoutTest {
+
+ // Configuration explicitly null
+ private static final Serializer serializer = new SerializerBuilder().setPattern(DefaultConfiguration.DEFAULT_PATTERN).build();
+
static class ConcreteStringLayout extends AbstractStringLayout {
public static int DEFAULT_STRING_BUILDER_SIZE = AbstractStringLayout.DEFAULT_STRING_BUILDER_SIZE;
public static int MAX_STRING_BUILDER_SIZE = AbstractStringLayout.MAX_STRING_BUILDER_SIZE;
public ConcreteStringLayout() {
- super(Charset.defaultCharset());
+ // Configuration explicitly null
+ super(null, Charset.defaultCharset(), serializer, serializer);
}
public static StringBuilder getStringBuilder() {
@@ -76,4 +86,14 @@ public class AbstractStringLayoutTest {
"capacity, trimmed to MAX_STRING_BUILDER_SIZE");
assertEquals(0, sb3.length(), "empty, ready for use");
}
+
+ @Test
+ public void testNullConfigurationIsAllowed() {
+ try {
+ final ConcreteStringLayout layout = new ConcreteStringLayout();
+ layout.serializeToString(serializer);
+ } catch (NullPointerException e) {
+ fail(e);
+ }
+ }
}
\ No newline at end of file
diff --git a/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java b/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
index ca0c141..4fc5bc9 100644
--- a/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
+++ b/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
@@ -23,13 +23,13 @@ import jakarta.servlet.ServletContext;
import org.apache.logging.log4j.core.Filter;
import org.apache.logging.log4j.core.Layout;
import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.StringLayout;
import org.apache.logging.log4j.core.appender.AbstractAppender;
import org.apache.logging.log4j.core.config.Property;
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
import org.apache.logging.log4j.core.layout.AbstractStringLayout;
-import org.apache.logging.log4j.core.layout.PatternLayout;
import org.apache.logging.log4j.web.WebLoggerContextUtils;
/**
@@ -55,10 +55,8 @@ public class ServletAppender extends AbstractAppender {
LOGGER.error("No servlet context is available");
return null;
}
- Layout<? extends Serializable> layout = getLayout();
- if (layout == null) {
- layout = PatternLayout.createDefaultLayout();
- } else if (!(layout instanceof AbstractStringLayout)) {
+ Layout<? extends Serializable> layout = getOrCreateLayout();
+ if (!(layout instanceof StringLayout)) {
LOGGER.error("Layout must be a StringLayout to log to ServletContext");
return null;
}
diff --git a/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java b/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
index b3c47e2..c9a70f6 100644
--- a/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
+++ b/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
@@ -23,13 +23,13 @@ import javax.servlet.ServletContext;
import org.apache.logging.log4j.core.Filter;
import org.apache.logging.log4j.core.Layout;
import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.StringLayout;
import org.apache.logging.log4j.core.appender.AbstractAppender;
import org.apache.logging.log4j.core.config.Property;
import org.apache.logging.log4j.core.config.plugins.Plugin;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory;
import org.apache.logging.log4j.core.layout.AbstractStringLayout;
-import org.apache.logging.log4j.core.layout.PatternLayout;
import org.apache.logging.log4j.web.WebLoggerContextUtils;
/**
@@ -55,10 +55,8 @@ public class ServletAppender extends AbstractAppender {
LOGGER.error("No servlet context is available");
return null;
}
- Layout<? extends Serializable> layout = getLayout();
- if (layout == null) {
- layout = PatternLayout.createDefaultLayout();
- } else if (!(layout instanceof AbstractStringLayout)) {
+ final Layout<? extends Serializable> layout = getOrCreateLayout();
+ if (!(layout instanceof StringLayout)) {
LOGGER.error("Layout must be a StringLayout to log to ServletContext");
return null;
}