From ef1ddcd4133951c4a370759a5364b277e586ca15 Mon Sep 17 00:00:00 2001 From: thisdudeiknew Date: Tue, 11 Jul 2023 13:58:37 -0400 Subject: [PATCH] Fix RollingFileManager and FileRenameAction to correctly propagate synchronous action failures --- .../rolling/RollingFileManagerTest.java | 51 +++++++++++++++++++ .../rolling/action/FileRenameActionTest.java | 12 +++-- .../appender/rolling/RollingFileManager.java | 2 +- .../rolling/action/FileRenameAction.java | 2 +- ...5_fix_synchronous_rolling_file_manager.xml | 27 ++++++++++ 5 files changed, 88 insertions(+), 6 deletions(-) create mode 100644 src/changelog/.2.x.x/1445_fix_synchronous_rolling_file_manager.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java index 796ad67a824..7da92ed5e23 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java @@ -21,7 +21,10 @@ import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.appender.RollingFileAppender; +import org.apache.logging.log4j.core.appender.rolling.action.AbstractAction; import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.NullConfiguration; +import org.apache.logging.log4j.core.layout.PatternLayout; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.core.util.IOUtils; import org.junit.Assert; @@ -84,4 +87,52 @@ public RolloverDescription rollover(final RollingFileManager manager) throws Sec } } } + + /** + * Test that a synchronous action failure does not cause a rollover. Addresses Issue #1445. + */ + @Test + public void testSynchronousActionFailure() throws IOException { + class FailingSynchronousAction extends AbstractAction { + @Override + public boolean execute() { + return false; + } + } + class FailingSynchronousStrategy implements RolloverStrategy { + @Override + public RolloverDescription rollover(final RollingFileManager manager) throws SecurityException { + return new RolloverDescriptionImpl(manager.getFileName(), false, new FailingSynchronousAction(), null); + } + } + + final Configuration configuration = new NullConfiguration(); + + // Create the manager. + final File file = File.createTempFile("testSynchronousActionFailure", "log"); + final RollingFileManager manager = RollingFileManager.getFileManager( + file.getAbsolutePath(), + "testSynchronousActionFailure.log.%d{yyyy-MM-dd}", true, false, + OnStartupTriggeringPolicy.createPolicy(1), new FailingSynchronousStrategy(), null, + PatternLayout.createDefaultLayout(), 0, true, false, null, null, null, configuration); + Assert.assertNotNull(manager); + manager.initialize(); + + // Get the initialTime of this original log file + final long initialTime = manager.getFileTime(); + + // Log something to ensure that the existing file size is > 0 + final String testContent = "Test"; + manager.writeToDestination(testContent.getBytes(StandardCharsets.US_ASCII), 0, testContent.length()); + + // Trigger rollover that will fail + manager.rollover(); + + // If the rollover fails, then the size should not be reset + Assert.assertNotEquals(0, manager.getFileSize()); + + // The initialTime should not have changed + Assert.assertEquals(initialTime, manager.getFileTime()); + + } } diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameActionTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameActionTest.java index 185baa2b648..ef3f3a0af1c 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameActionTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameActionTest.java @@ -46,7 +46,8 @@ public void testRename1() throws Exception { final File dest = new File(tempDir, "newFile.log"); final FileRenameAction action = new FileRenameAction(file, dest, false); - action.execute(); + boolean renameResult = action.execute(); + assertTrue(renameResult, "Rename action returned false"); assertTrue(dest.exists(), "Renamed file does not exist"); assertFalse(file.exists(), "Old file exists"); } @@ -60,7 +61,8 @@ public void testEmpty() throws Exception { assertTrue(file.exists(), "File to rename does not exist"); final File dest = new File(tempDir, "newFile.log"); final FileRenameAction action = new FileRenameAction(file, dest, false); - action.execute(); + boolean renameResult = action.execute(); + assertTrue(renameResult, "Rename action returned false"); assertFalse(dest.exists(), "Renamed file should not exist"); assertFalse(file.exists(), "Old file still exists"); } @@ -74,7 +76,8 @@ public void testRenameEmpty() throws Exception { assertTrue(file.exists(), "File to rename does not exist"); final File dest = new File(tempDir, "newFile.log"); final FileRenameAction action = new FileRenameAction(file, dest, true); - action.execute(); + boolean renameResult = action.execute(); + assertTrue(renameResult, "Rename action returned false"); assertTrue(dest.exists(), "Renamed file should exist"); assertFalse(file.exists(), "Old file still exists"); } @@ -91,7 +94,8 @@ public void testNoParent() throws Exception { final File dest = new File(tempDir, "newFile.log"); final FileRenameAction action = new FileRenameAction(file, dest, false); - action.execute(); + boolean renameResult = action.execute(); + assertTrue(renameResult, "Rename action returned false"); assertTrue(dest.exists(), "Renamed file does not exist"); assertFalse(file.exists(), "Old file exists"); } 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 0a86005eace..0ceef8056f1 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 @@ -520,7 +520,7 @@ private boolean rollover(final RolloverStrategy strategy) { asyncExecutor.execute(new AsyncAction(descriptor.getAsynchronous(), this)); releaseRequired = false; } - return true; + return success; } return false; } finally { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameAction.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameAction.java index a42b5af02e8..90be9dda80f 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameAction.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/FileRenameAction.java @@ -163,7 +163,7 @@ public static boolean execute(final File source, final File destination, final b } } else { try { - source.delete(); + return source.delete(); } catch (final Exception exDelete) { LOGGER.error("Unable to delete empty file {}: {} {}", source.getAbsolutePath(), exDelete.getClass().getName(), exDelete.getMessage()); diff --git a/src/changelog/.2.x.x/1445_fix_synchronous_rolling_file_manager.xml b/src/changelog/.2.x.x/1445_fix_synchronous_rolling_file_manager.xml new file mode 100644 index 00000000000..4e446ebf50b --- /dev/null +++ b/src/changelog/.2.x.x/1445_fix_synchronous_rolling_file_manager.xml @@ -0,0 +1,27 @@ + + + + + + + Fixed `RollingFileManager` to propagate failed synchronous actions correctly. + +