From f78729386a2c31e2e0b25ccfd81e19f0a906c7db Mon Sep 17 00:00:00 2001 From: Ian Pickering Date: Thu, 25 Apr 2019 15:40:45 -0700 Subject: [PATCH] Insert missing files in DefaultRolloverStrategy if a higher numbered file exists already --- .../rolling/DefaultRolloverStrategy.java | 39 +++++++++- ...penderMissingNumberedLogsOverflowTest.java | 78 +++++++++++++++++++ ...ollingAppenderMissingNumberedLogsTest.java | 75 ++++++++++++++++++ .../log4j-missing-numbered-logs-overflow.xml | 37 +++++++++ .../resources/log4j-missing-numbered-logs.xml | 37 +++++++++ 5 files changed, 263 insertions(+), 3 deletions(-) create mode 100644 log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsOverflowTest.java create mode 100644 log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsTest.java create mode 100644 log4j-core/src/test/resources/log4j-missing-numbered-logs-overflow.xml create mode 100644 log4j-core/src/test/resources/log4j-missing-numbered-logs.xml diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java index 1c21474cc..421c99313 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DefaultRolloverStrategy.java @@ -22,6 +22,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.SortedMap; @@ -371,12 +372,27 @@ public class DefaultRolloverStrategy extends AbstractRolloverStrategy { break; } } + HashSet takenIndices = new HashSet<>(); + int lastIndex = 0; + final StringBuilder buf = new StringBuilder(); if (renameFiles) { for (final Map.Entry entry : eligibleFiles.entrySet()) { buf.setLength(0); + int index = Math.min(entry.getKey() - 1, highIndex); + + if (!takenIndices.contains(lastIndex + 1)) { + index = lastIndex + 1; + } + else if (takenIndices.contains(index)) { + index = entry.getKey() - 1; + } + + takenIndices.add(index); + lastIndex = index; + // LOG4J2-531: directory scan & rollover must use same format - manager.getPatternProcessor().formatFileName(strSubstitutor, buf, entry.getKey() - 1); + manager.getPatternProcessor().formatFileName(strSubstitutor, buf, index); final String currentName = entry.getValue().toFile().getName(); String renameTo = buf.toString(); final int suffixLength = suffixLength(renameTo); @@ -396,8 +412,25 @@ public class DefaultRolloverStrategy extends AbstractRolloverStrategy { } } - return eligibleFiles.size() > 0 ? - (eligibleFiles.lastKey() < highIndex ? eligibleFiles.lastKey() + 1 : highIndex) : lowIndex; + int index = lowIndex; + + if (eligibleFiles.size() > 0) { + index = highIndex; + + if (eligibleFiles.lastKey() < highIndex) { + index = eligibleFiles.lastKey() + 1; + } + } + + // LOG4J2-2595: If there are any missing files, prioritize renaming to the one with the lowest number. + for (int i = lowIndex; i < highIndex; i++) { + if (!eligibleFiles.containsKey(i)) { + index = i; + break; + } + } + + return index; } /** diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsOverflowTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsOverflowTest.java new file mode 100644 index 000000000..bab84db78 --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsOverflowTest.java @@ -0,0 +1,78 @@ +package org.apache.logging.log4j.core.appender.rolling; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.junit.LoggerContextRule; +import org.apache.logging.log4j.status.StatusLogger; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; + +/** + * Test LOG4J2-2595. + */ +public class RollingAppenderMissingNumberedLogsOverflowTest { + + private static final String CONFIG = "log4j-missing-numbered-logs-overflow.xml"; + + private static final String DIR = "target/missing-numbered-logs-overflow"; + + private static final String FILE = "missing-numbered-logs-overflow-test.log"; + + @Rule + public LoggerContextRule loggerContextRule = LoggerContextRule + .createShutdownTimeoutLoggerContextRule(CONFIG); + + private static void deleteIfExists(String file) throws Exception { + Path log = Paths.get(DIR, file); + if (Files.exists(log)) { + Files.delete(log); + } + } + + private static void createFile(String file) throws Exception { + Path log = Paths.get(DIR, file); + Files.createDirectories(log.getParent()); + Files.createFile(log); + } + + @BeforeClass + public static void beforeClass() throws Exception { + deleteIfExists(FILE); + + for (int i=1; i < 10; ++i) { + deleteIfExists(FILE + "." + i); + } + + for (int i=1; i < 20; ++i) { + deleteIfExists(FILE + "." + (10000 + i)); + createFile(FILE + "." + (10000 + i)); + } + } + + @Test + public void testAppender() throws Exception { + Logger logger = loggerContextRule.getRootLogger(); + + // Trigger a single rollover + logger.info(StringUtils.repeat("a", 1024)); + logger.info(StringUtils.repeat("a", 1024)); + + Assert.assertTrue(FILE + " doesn't exist", Files.exists(Paths.get(DIR, FILE))); + + for (int i=1; i < 10; ++i) { + Assert.assertTrue(FILE + "." + i + " doesn't exist", Files.exists(Paths.get(DIR, FILE + "." + i))); + } + + Assert.assertFalse(FILE + ".10 exists", Files.exists(Paths.get(DIR, FILE + ".10"))); + + for (int i=1; i < 10; ++i) { + Assert.assertFalse(FILE + "." + (10000 + i) + " exists", Files.exists(Paths.get(DIR, FILE + "." + (10000 + i)))); + } + } +} diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsTest.java new file mode 100644 index 000000000..758ccc331 --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingAppenderMissingNumberedLogsTest.java @@ -0,0 +1,75 @@ +package org.apache.logging.log4j.core.appender.rolling; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.junit.LoggerContextRule; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Rule; +import org.junit.Test; + +/** + * Test LOG4J2-2595. + */ +public class RollingAppenderMissingNumberedLogsTest { + + private static final String CONFIG = "log4j-missing-numbered-logs.xml"; + + private static final String DIR = "target/missing-numbered-logs"; + + private static final String FILE = "missing-numbered-logs-test.log"; + + @Rule + public LoggerContextRule loggerContextRule = LoggerContextRule + .createShutdownTimeoutLoggerContextRule(CONFIG); + + private static void deleteIfExists(String file) throws Exception { + Path log = Paths.get(DIR, file); + if (Files.exists(log)) { + Files.delete(log); + } + } + + private static void createFile(String file) throws Exception { + Path log = Paths.get(DIR, file); + Files.createDirectories(log.getParent()); + Files.createFile(log); + } + + @BeforeClass + public static void beforeClass() throws Exception { + deleteIfExists(FILE + ".10000"); + deleteIfExists(FILE + ".10001"); + deleteIfExists(FILE); + + for (int i=1; i < 10; ++i) { + deleteIfExists(FILE + "." + i); + } + + createFile(FILE + ".10000"); + createFile(FILE + ".10001"); + } + + @Test + public void testAppender() throws Exception { + Logger logger = loggerContextRule.getRootLogger(); + + // Trigger a single rollover + for (int i = 0; i < 10; i++) { + logger.info(StringUtils.repeat("a", 1024)); + } + + Assert.assertTrue(FILE + " doesn't exist", Files.exists(Paths.get(DIR, FILE))); + + for (int i=1; i < 10; ++i) { + Assert.assertTrue(FILE + "." + i + " doesn't exist", Files.exists(Paths.get(DIR, FILE + "." + i))); + } + + Assert.assertFalse(FILE + ".10 exists", Files.exists(Paths.get(DIR, FILE + ".10"))); + Assert.assertFalse(FILE + ".10000 exists", Files.exists(Paths.get(DIR, FILE + ".10000"))); + Assert.assertFalse(FILE + ".10001 exists", Files.exists(Paths.get(DIR, FILE + ".10001"))); + } +} diff --git a/log4j-core/src/test/resources/log4j-missing-numbered-logs-overflow.xml b/log4j-core/src/test/resources/log4j-missing-numbered-logs-overflow.xml new file mode 100644 index 000000000..b1f1d664a --- /dev/null +++ b/log4j-core/src/test/resources/log4j-missing-numbered-logs-overflow.xml @@ -0,0 +1,37 @@ + + + + + + + %d %p %c{1.} [%t] %m%n + + + + + + + + + + + + + \ No newline at end of file diff --git a/log4j-core/src/test/resources/log4j-missing-numbered-logs.xml b/log4j-core/src/test/resources/log4j-missing-numbered-logs.xml new file mode 100644 index 000000000..20cb2d646 --- /dev/null +++ b/log4j-core/src/test/resources/log4j-missing-numbered-logs.xml @@ -0,0 +1,37 @@ + + + + + + + %d %p %c{1.} [%t] %m%n + + + + + + + + + + + + + \ No newline at end of file -- 2.20.1.windows.1