Fix RollingFileManager and FileRenameAction to correctly propagate synchronous action failures

Author: thisdudeiknew

log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java

@@ -16,17 +16,24 @@
  */
 package org.apache.logging.log4j.core.appender.rolling;
 
-import java.io.*;
-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;
 import org.junit.Test;
 
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.Reader;
+import java.nio.charset.StandardCharsets;
+
 public class RollingFileManagerTest {
 
     /**
@@ -84,4 +91,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());
+
+    }
 }

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");
     }

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 {

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());

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>