This is an automated email from the ASF dual-hosted git repository. vy pushed a commit to branch release/2.21.0 in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 66bc65079cb437ac06da8ab7e09ce9bbf75838fe Author: Tim P <[email protected]> AuthorDate: Thu Oct 5 09:10:38 2023 -0400 Propagate synchronous action failures in `RollingFileManager` and `FileRenameAction` (#1445, #1549) --- .../appender/rolling/RollingFileManagerTest.java | 51 ++++++++++++++++++++++ .../rolling/action/FileRenameActionTest.java | 12 +++-- .../core/appender/rolling/RollingFileManager.java | 2 +- .../appender/rolling/action/FileRenameAction.java | 2 +- .../1445_fix_synchronous_rolling_file_manager.xml | 27 ++++++++++++ 5 files changed, 88 insertions(+), 6 deletions(-) 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 796ad67a82..7da92ed5e2 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 java.nio.charset.StandardCharsets; 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 class RollingFileManagerTest { } } } + + /** + * 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 185baa2b64..ef3f3a0af1 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 class FileRenameActionTest { 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 class FileRenameActionTest { 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 class FileRenameActionTest { 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 class FileRenameActionTest { 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 0a86005eac..0ceef8056f 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 @@ public class RollingFileManager extends FileManager { 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 a42b5af02e..90be9dda80 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 class FileRenameAction extends AbstractAction { } } 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 0000000000..4e446ebf50 --- /dev/null +++ b/src/changelog/.2.x.x/1445_fix_synchronous_rolling_file_manager.xml @@ -0,0 +1,27 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ 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. + --> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="http://logging.apache.org/log4j/changelog"; + xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.1.xsd"; + type="fixed"> + <issue id="1445" link="https://github.com/apache/logging-log4j2/issues/1445"/> + <author id="thisdudeiknew" name="Timothy Pfeifer"/> + <description format="asciidoc"> + Fixed `RollingFileManager` to propagate failed synchronous actions correctly. + </description> +</entry>
