This is an automated email from the ASF dual-hosted git repository. rgoers pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/master by this push: new 1f046aa LOG4j2-2061 - Use the file pattern as the FileManager name when no filename is present. 1f046aa is described below commit 1f046aa19f9a885be308bf6e96d56b31c2cce393 Author: Ralph Goers <rgo...@apache.org> AuthorDate: Sat Feb 2 16:36:00 2019 -0700 LOG4j2-2061 - Use the file pattern as the FileManager name when no filename is present. --- .../core/appender/rolling/RollingFileManager.java | 4 +- .../core/appender/ReconfigureAppenderTest.java | 170 +++++++++++++++++++++ src/changes/changes.xml | 3 + 3 files changed, 175 insertions(+), 2 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java index 1f4799f..2863e63 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java @@ -94,8 +94,8 @@ public class RollingFileManager extends FileManager { final String advertiseURI, final Layout<? extends Serializable> layout, final String filePermissions, final String fileOwner, final String fileGroup, final boolean writeHeader, final ByteBuffer buffer) { - super(loggerContext, fileName, os, append, false, createOnDemand, advertiseURI, layout, - filePermissions, fileOwner, fileGroup, writeHeader, buffer); + super(loggerContext, fileName != null ? fileName : pattern, os, append, false, createOnDemand, + advertiseURI, layout, filePermissions, fileOwner, fileGroup, writeHeader, buffer); this.size = size; this.initialTime = initialTime; this.triggeringPolicy = triggeringPolicy; diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/ReconfigureAppenderTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/ReconfigureAppenderTest.java new file mode 100644 index 0000000..1df2519 --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/ReconfigureAppenderTest.java @@ -0,0 +1,170 @@ +/* + * Copyright (c) 2019 Nextiva, Inc. to Present. + * All rights reserved. + */ +package org.apache.logging.log4j.core.appender; + +import java.lang.reflect.Field; +import java.util.Map; + +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; + +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.rolling.DirectWriteRolloverStrategy; +import org.apache.logging.log4j.core.appender.rolling.SizeBasedTriggeringPolicy; +import org.apache.logging.log4j.core.config.Configurator; +import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder; +import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFactory; +import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration; +import org.apache.logging.log4j.core.layout.PatternLayout; +import org.apache.logging.log4j.core.util.Builder; +import org.junit.Test; + +/** + * Class Description goes here. + * Created by rgoers on 2019-02-02 + */ +public class ReconfigureAppenderTest { + private RollingFileAppender appender; + + @Test + public void addAndRemoveAppenderTest() + { + // this will create a rolling file appender and add it to the logger + // of this class. The file manager is created for the first time. + // see AbstractManager.getManager(...). + this.createAndAddAppender(); + + // let's write something to the logger to ensure the output stream is opened. + // We expect this call to create a a new output stream (which is does). + // see OutputStreamManager.writeToDestination(...). + Logger logger = (Logger)LogManager.getLogger(this.getClass()); + logger.info("test message 1"); + + // this will close the rolling file appender and remove it from the logger + // of this class. We expect the file output stream to be closed (which is it) + // however the FileManager instance is kept in AbstractManager.MAP. This means that + // when we create a new rolling file appender with the DirectWriteRolloverStrategy + // this OLD file manager will be retrieved from the map (since it has the SAME file pattern) + // and this is a problem as the output stream on that file manager is CLOSED. The problem + // here is that we attempt to remove a file manager call NULL instead of FILE PATTERN + this.removeAppender(); + + // create a new rolling file appender for this logger. We expect this to create a new file + // manager as the old one should have been removed. Since the instance of this file manager + // is still in AbstractManager.MAP, it is returned and assigned to our new rolling file + // appender instance. The problem here is that the file manager is create with the name + // FILE PATTERN and that its output stream is closed. + this.createAndAddAppender(); + + // try and log something. This will not be logged anywhere. An exception will be thrown: + // Caused by: java.io.IOException: Stream Closed + logger.info("test message 2"); + + // remove the appender as before. + this.removeAppender(); + + // this method will use reflection to go and remove the instance of FileManager from the AbstractManager.MAP + // ourselves. This means that the rolling file appender has been stopped (previous method) AND its + // manager has been removed. + this.removeManagerUsingReflection(); + + // now that the instance of FileManager is not present in MAP, creating the appender will actually + // create a new rolling file manager, and put this in the map (keyed on file pattern again). + this.createAndAddAppender(); + + // because we have a new instance of file manager, this will create a new output stream. We can verify + // this by looking inside the filepattern.1.log file inside the working directory, and noticing that + // we have 'test message 1' followed by 'test message 3'. 'test message 2' is missing because we attempted + // to write while the output stream was closed. + logger.info("test message 3"); + + // possible fixes: + // 1) create the RollingFileManager and set it's name to FILE PATTERN when using DirectWriteRolloverStrategy + // 2) when stopping the appender (and thus the manager), remove on FILE PATTERN if DirectWriteRolloverStrategy + // 3) on OutputStreamManager.getOutputStream(), determine if the output stream is closed, and if it is create + // a new one. Note that this isn't really desirable as the only fix as if the file pattern had to change + // an instance of file manager would still exist in MAP, causing a resource leak. + + // now the obvious problem here is that if multiple file appenders use the same rolling file manager. We may run + // into a case where the file manager is removed and the output stream is closed, and the remaining appenders + // may not work correctly. I'm not sure of the use case in this scenario, and if people actually do this + // but based on the code it would be possible. I have also not tested this scenario out as it is not the + // scenario we would ever use, but it should be considered while fixing this issue. + } + private void removeManagerUsingReflection() + { + try + { + Field field = AbstractManager.class.getDeclaredField("MAP"); + field.setAccessible(true); + + // Retrieve the map itself. + Map<String, AbstractManager> map = + (Map<String, AbstractManager>)field.get(null); + + // Remove the file manager keyed on file pattern. + map.remove(appender.getFilePattern()); + } + catch (Exception e) + { + e.printStackTrace(); + } + } + + private void removeAppender() + { + Logger logger = (Logger)LogManager.getLogger(this.getClass()); + + // This call attempts to remove the file manager, but uses the name of the appender + // (NULL in this case) instead of PATTERN. + // see AbstractManager.stop(...). + appender.stop(); + logger.removeAppender(appender); + } + + private void createAndAddAppender() + { + ConfigurationBuilder<BuiltConfiguration> config_builder = + ConfigurationBuilderFactory.newConfigurationBuilder(); + + // All loggers must have a root logger. The default root logger logs ERRORs to the console. + // Override this with a root logger that does not log anywhere as we leave it up the the + // appenders on the logger. + config_builder.add(config_builder.newRootLogger(Level.INFO)); + + // Initialise the logger context. + LoggerContext logger_context = + Configurator.initialize(config_builder.build()); + + // Retrieve the logger. + Logger logger = (Logger) LogManager.getLogger(this.getClass()); + + Builder pattern_builder = PatternLayout.newBuilder().withPattern( + "[%d{dd-MM-yy HH:mm:ss}] %p %m %throwable %n"); + + PatternLayout pattern_layout = (PatternLayout) pattern_builder.build(); + + appender = RollingFileAppender + .newBuilder() + .withLayout(pattern_layout) + .withName("rollingfileappender") + .withFilePattern("target/filepattern.%i.log") + .withPolicy(SizeBasedTriggeringPolicy.createPolicy("5 MB")) + .withAppend(true) + .withStrategy( + DirectWriteRolloverStrategy + .newBuilder() + .withConfig(logger_context.getConfiguration()) + .withMaxFiles("5") + .build()) + .setConfiguration(logger_context.getConfiguration()) + .build(); + + appender.start(); + + logger.addAppender(appender); + } +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index ca2862c..b52d4a0 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -354,6 +354,9 @@ </action> </release> <release version="2.11.2" date="2018-MM-DD" description="GA Release 2.11.2"> + <action issue="LOG4J2-2061" dev="rgoers" type="fix"> + Use the file pattern as the FileManager "name" when no filename is present. + </action> <action issue="LOG4J2-2009" dev="rgoers" type="fix"> Expose LoggerContext.setConfiguration as a public method. </action>