Reopen log file when rollover is unsuccessful (#3226)

Fixes `RollingFileManager` to reopen the log file when the rollover was unsuccessful

Author: king-tyler

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

@@ -17,17 +17,21 @@
 package org.apache.logging.log4j.core.appender.rolling;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.io.OutputStream;
 import java.io.Reader;
+import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
 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;
@@ -138,21 +142,18 @@ public RolloverDescription rollover(final RollingFileManager manager) throws Sec
         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
-        assertNotEquals(0, manager.getFileSize());
+        // If the rollover fails, then the log file should be unchanged
+        assertEquals(file.getAbsolutePath(), manager.getFileName());
 
-        // The initialTime should not have changed
-        assertEquals(initialTime, manager.getFileTime());
+        // The logged content should be unchanged
+        assertEquals(testContent, new String(Files.readAllBytes(file.toPath()), StandardCharsets.US_ASCII));
     }
 
     @Test
@@ -188,4 +189,39 @@ public void testCreateParentDir() {
             manager.close();
         }
     }
+
+    @Test
+    @Issue("https://github.com/apache/logging-log4j2/issues/2592")
+    public void testRolloverOfDeletedFile() throws IOException {
+        final File file = File.createTempFile("testRolloverOfDeletedFile", "log");
+        file.deleteOnExit();
+        final String testContent = "Test";
+        try (final OutputStream os =
+                        new ByteArrayOutputStream(); // use a dummy OutputStream so that the real file can be deleted
+                final RollingFileManager manager = new RollingFileManager(
+                        null,
+                        file.getAbsolutePath(),
+                        "testRolloverOfDeletedFile.log.%d{yyyy-MM-dd}",
+                        os,
+                        true,
+                        false,
+                        0,
+                        System.currentTimeMillis(),
+                        OnStartupTriggeringPolicy.createPolicy(1),
+                        DefaultRolloverStrategy.newBuilder().build(),
+                        file.getName(),
+                        null,
+                        null,
+                        null,
+                        null,
+                        false,
+                        ByteBuffer.allocate(256))) {
+            assertTrue(file.delete());
+            manager.setRenameEmptyFiles(true);
+            manager.rollover();
+            assertEquals(file.getAbsolutePath(), manager.getFileName());
+            manager.writeBytes(testContent.getBytes(StandardCharsets.US_ASCII), 0, testContent.length());
+        }
+        assertEquals(testContent, new String(Files.readAllBytes(file.toPath()), StandardCharsets.US_ASCII));
+    }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManager.java

@@ -604,43 +604,43 @@ public RolloverStrategy getRolloverStrategy() {
 
     private boolean rollover(final RolloverStrategy strategy) {
 
-        boolean releaseRequired = false;
+        boolean outputStreamClosed = false;
         try {
             // Block until the asynchronous operation is completed.
             semaphore.acquire();
-            releaseRequired = true;
         } catch (final InterruptedException e) {
             logError("Thread interrupted while attempting to check rollover", e);
-            return false;
+            return outputStreamClosed;
         }
 
-        boolean success = true;
+        boolean asyncActionStarted = true;
 
         try {
             final RolloverDescription descriptor = strategy.rollover(this);
             if (descriptor != null) {
                 writeFooter();
                 closeOutputStream();
+                outputStreamClosed = true;
+                boolean syncActionSuccess = true;
                 if (descriptor.getSynchronous() != null) {
                     LOGGER.debug("RollingFileManager executing synchronous {}", descriptor.getSynchronous());
                     try {
-                        success = descriptor.getSynchronous().execute();
+                        syncActionSuccess = descriptor.getSynchronous().execute();
                     } catch (final Exception ex) {
-                        success = false;
+                        syncActionSuccess = false;
                         logError("Caught error in synchronous task", ex);
                     }
                 }
 
-                if (success && descriptor.getAsynchronous() != null) {
+                if (syncActionSuccess && descriptor.getAsynchronous() != null) {
                     LOGGER.debug("RollingFileManager executing async {}", descriptor.getAsynchronous());
                     asyncExecutor.execute(new AsyncAction(descriptor.getAsynchronous(), this));
-                    releaseRequired = false;
+                    asyncActionStarted = false;
                 }
-                return success;
             }
-            return false;
+            return outputStreamClosed;
         } finally {
-            if (releaseRequired) {
+            if (asyncActionStarted) {
                 semaphore.release();
             }
         }

src/changelog/.2.x.x/2592_fix_RollingFileManager_unsuccessful_rollover.xml

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="2592" link="https://github.com/apache/logging-log4j2/issues/2592"/>
+  <description format="asciidoc">Fix `RollingFileManager` to reopen the log file when the rollover was unsuccessful</description>
+</entry>