diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java index 65f3ccb910f..37a97c6cd66 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicyTest.java @@ -16,13 +16,17 @@ */ package org.apache.logging.log4j.core.appender.rolling; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.text.SimpleDateFormat; +import java.util.Date; import org.apache.logging.log4j.core.appender.RollingFileAppender; import org.apache.logging.log4j.core.appender.RollingRandomAccessFileAppender; import org.apache.logging.log4j.core.config.Configurator; import org.apache.logging.log4j.core.config.NullConfiguration; import org.apache.logging.log4j.core.layout.PatternLayout; +import org.apache.logging.log4j.core.util.CronExpression; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -72,6 +76,30 @@ void testBuilderOnce() { testBuilder(); } + /** + * Without a {@code fileName} the appender writes directly to the file its pattern resolves to. + * That file covers the whole rollover period, so it must be named after the period's start + * rather than after the moment the appender happened to start. Otherwise a restart later in + * the period opens a second file for a period that is supposed to have only one. + */ + @Test + void testDirectWriteFileNameUsesPeriodStart() throws Exception { + final String schedule = "0 0 0 ? * SUN"; + // @formatter:off + final RollingFileAppender raf = RollingFileAppender.newBuilder() + .setName("test5") + .setFilePattern("target/testcmd5.log-%d{yyyyMMdd}") + .setPolicy(CronTriggeringPolicy.createPolicy(configuration, Boolean.FALSE.toString(), schedule)) + .setConfiguration(configuration) + .build(); + // @formatter:on + assertNotNull(raf); + + final Date periodStart = new CronExpression(schedule).getPrevFireTime(new Date()); + final String expected = "target/testcmd5.log-" + new SimpleDateFormat("yyyyMMdd").format(periodStart); + assertEquals(expected, raf.getManager().getFileName()); + } + /** * Tests LOG4J2-1740 Add CronTriggeringPolicy programmatically leads to NPE */ diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java index 7ec778764e1..cd99b371762 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/CronTriggeringPolicy.java @@ -150,7 +150,9 @@ private static CronExpression getSchedule(final String expression) { private void rollover() { // If possible, use the time rollover was supposed to occur, not the actual time. final Date rollTime = future != null ? future.getFireTime() : new Date(); - manager.rollover(cronExpression.getPrevFireTime(rollTime), lastRollDate); + // The file being rolled covers the period that ends at `rollTime`, so it is named after + // that period's start. The file replacing it opens the period beginning at `rollTime`. + manager.rollover(cronExpression.getPrevFireTime(rollTime), rollTime); if (future != null) { lastRollDate = future.getFireTime(); } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java index e105fa455b1..59600b69e74 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/DirectWriteRolloverStrategy.java @@ -390,8 +390,10 @@ public String getCurrentFileName(final RollingFileManager manager) { final SortedMap eligibleFiles = getEligibleFiles(manager); final int fileIndex = eligibleFiles.size() > 0 ? (nextIndex > 0 ? nextIndex : eligibleFiles.lastKey()) : 1; final StringBuilder buf = new StringBuilder(255); - // LOG4J2-3339 - Always use the current time for new direct write files. - manager.getPatternProcessor().setCurrentFileTime(System.currentTimeMillis()); + // Name the file after the start of the rollover period it belongs to, which the + // triggering policy records as the pattern processor's current file time. Policies + // that do not track a period leave that value at 0, and `formatFileName()` then + // falls back to the current time on its own. manager.getPatternProcessor().formatFileName(strSubstitutor, buf, true, fileIndex); final int suffixLength = suffixLength(buf.toString()); final String name = suffixLength > 0 ? buf.substring(0, buf.length() - suffixLength) : buf.toString(); diff --git a/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml new file mode 100644 index 00000000000..cb4271a1329 --- /dev/null +++ b/src/changelog/.2.x.x/fix_direct_write_cron_current_file_name.xml @@ -0,0 +1,15 @@ + + + + + Fix the file a `CronTriggeringPolicy` appender without a `fileName` writes to. The file is now named after + the start of the rollover period it covers, instead of after the moment the appender started, so restarting + within a period reopens that period's file rather than creating another one. Note that this changes the + names of direct write files for such appenders. + +