This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 10.1.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 6baf03eb9307f13e6ed62846e75338869023614a Author: opencode <[email protected]> AuthorDate: Tue Oct 20 09:54:42 2026 +0200 Fix recovery after write failures in JULI FileHandler A failure while writing a record was reported through the error manager but the writer was never reset. Since PrintWriter swallows the exceptions of the underlying stream instead of propagating them, every later flush reported nothing either, and the handler stayed attached to a writer that could no longer complete writes, silently losing all further records for the lifetime of the handler. The error flag is now checked explicitly after the write through flush of the unbuffered mode and a failed writer is discarded, so the next record reopens the file and logging recovers on its own. closeWriter() had a related problem. Writing the tail, flushing or closing could fail, in which case the writer field was left set. Every subsequent close() then repeated the failure report for the same writer, which was never released. The writer is now closed and discarded unconditionally, with a failure of the tail and a failure of the close each reported at most once. Added regression tests that fail a write through /dev/full and a tail that throws, asserting the number of reports and that a failed writer is not carried over. --- java/org/apache/juli/FileHandler.java | 38 +++++++++--- test/org/apache/juli/TestFileHandler.java | 96 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 7 deletions(-) diff --git a/java/org/apache/juli/FileHandler.java b/java/org/apache/juli/FileHandler.java index 56de314fe3..48089272b7 100644 --- a/java/org/apache/juli/FileHandler.java +++ b/java/org/apache/juli/FileHandler.java @@ -275,6 +275,13 @@ public class FileHandler extends Handler { writer.write(result); if (bufferSize.intValue() < 0) { writer.flush(); + // PrintWriter swallows the exceptions from the underlying stream instead of + // propagating them, so the error flag needs to be checked explicitly after the + // flush. Throwing below discards the writer and lets a later record open a new + // one, as the next record will find the date unset and re-open the file. + if (writer.checkError()) { + throw new IOException("Unable to write to the log file"); + } } } else { reportError("FileHandler is closed or not yet initialized, unable to log [" + result + "]", null, @@ -282,6 +289,14 @@ public class FileHandler extends Handler { } } catch (Exception e) { reportError(null, e, ErrorManager.WRITE_FAILURE); + // Discard the writer, so the next record attempts to reopen the file rather than + // remaining attached to a writer that can no longer complete writes + writerLock.readLock().unlock(); + try { + closeWriter(); + } finally { + writerLock.readLock().lock(); + } } } finally { writerLock.readLock().unlock(); @@ -310,13 +325,22 @@ public class FileHandler extends Handler { if (writer == null) { return; } - writer.write(getFormatter().getTail(this)); - writer.flush(); - writer.close(); - writer = null; - date = null; - } catch (Exception e) { - reportError(null, e, ErrorManager.CLOSE_FAILURE); + try { + writer.write(getFormatter().getTail(this)); + writer.flush(); + } catch (Exception e) { + reportError(null, e, ErrorManager.CLOSE_FAILURE); + } finally { + // The writer is closed and discarded even when the tail could not be written, so a + // failure is not repeated by every later close() for a writer that is never released + try { + writer.close(); + } catch (Exception e) { + reportError(null, e, ErrorManager.CLOSE_FAILURE); + } + writer = null; + date = null; + } } finally { writerLock.writeLock().unlock(); } diff --git a/test/org/apache/juli/TestFileHandler.java b/test/org/apache/juli/TestFileHandler.java index 7b7d9725c5..30c83e735d 100644 --- a/test/org/apache/juli/TestFileHandler.java +++ b/test/org/apache/juli/TestFileHandler.java @@ -23,11 +23,15 @@ import java.nio.file.Files; import java.nio.file.Path; import java.time.LocalDate; import java.time.LocalDateTime; +import java.util.logging.ErrorManager; +import java.util.logging.Formatter; +import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.LogRecord; import org.junit.After; import org.junit.Assert; +import org.junit.Assume; import org.junit.Before; import org.junit.Test; @@ -166,6 +170,98 @@ public class TestFileHandler { return count; } + /* + * A write failure must be reported, and the failed writer must be discarded so + * that the next write retries opening the file rather than silently writing into + * the void for the lifetime of the handler. + */ + @Test + public void testWriterDiscardedOnWriteFailure() throws Exception { + Assume.assumeTrue("Requires /dev/full", new File("/dev/full").canWrite()); + + // Buffer size below zero means: write through, no buffering + FileHandler handler = new FileHandler("/dev/", "full", "", Integer.valueOf(-1), + Boolean.FALSE, Integer.valueOf(-1)); + try { + CountingErrorManager errorManager = new CountingErrorManager(); + handler.setErrorManager(errorManager); + + // Neither of these can succeed. PrintWriter swallows the resulting + // IOExceptions, so a check is needed after the flush. + handler.publish(createRecord("first")); + handler.publish(createRecord("second")); + + // Opening /dev/full itself must succeed. Otherwise the records would + // be reported as failures without the write path ever being exercised. + Assert.assertEquals(0, errorManager.openFailures); + + // Before the fix the failures were swallowed and never reported (zero + // here). After the fix each failed write is reported once. + Assert.assertEquals(2, errorManager.writeFailures); + + // The failed writer must not be re-used: closing it must succeed (it is + // a writer to /dev/full), so no further failures are reported. + handler.close(); + Assert.assertEquals(0, errorManager.closeFailures); + } finally { + handler.close(); + } + } + + /* + * If the tail cannot be written or the writer cannot be closed, close() must + * still discard the writer. Otherwise every subsequent close() repeats the + * failure report for a writer that is never released. + */ + @Test + public void testWriterDiscardedOnTailFailure() throws Exception { + FileHandler handler = new FileHandler(logsDir.getAbsolutePath(), "tail", "", + Integer.valueOf(-1), Boolean.FALSE, Integer.valueOf(-1)); + handler.setFormatter(new Formatter() { + @Override + public String format(LogRecord record) { + return record.getMessage() + System.lineSeparator(); + } + + @Override + public String getTail(Handler h) { + throw new IllegalStateException("Test tail failure"); + } + }); + CountingErrorManager errorManager = new CountingErrorManager(); + handler.setErrorManager(errorManager); + + handler.publish(createRecord("message")); + handler.close(); + // Before the fix the writer was left open, so this reported a second time. + handler.close(); + + Assert.assertEquals(1, errorManager.closeFailures); + } + + private static LogRecord createRecord(String message) { + LogRecord record = new LogRecord(Level.INFO, message); + record.setLoggerName("test"); + return record; + } + + private static final class CountingErrorManager extends ErrorManager { + private int closeFailures; + private int writeFailures; + private int openFailures; + + @Override + public void error(String msg, Exception ex, int code) { + if (code == ErrorManager.CLOSE_FAILURE) { + closeFailures++; + } else if (code == ErrorManager.WRITE_FAILURE) { + writeFailures++; + } else if (code == ErrorManager.OPEN_FAILURE) { + openFailures++; + } + } + } + private void generateLogFiles(File dir, String prefix, String suffix, int amount) throws IOException { for (int i = 0; i < amount; i++) { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
