You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by pk...@apache.org on 2023/10/05 19:12:42 UTC
[logging-log4j2] 05/11: Propagate synchronous action failures in `RollingFileManager` and `FileRenameAction` (#1445, #1549)
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch release/2.21.0
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 6dda4ae33f9681dffed394510ce37b6614386631
Author: Tim P <17...@users.noreply.github.com>
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>