From d65450092405bddb7475f07976a1594610d6410f Mon Sep 17 00:00:00 2001 From: Roman Shevchenko Date: Tue, 28 Nov 2023 22:06:29 +0100 Subject: [PATCH] [platform] no log write amplification in `RollingFileHandler` (IDEA-339256) GitOrigin-RevId: 36e7843813c16cb30a0ef9537834d6d59a1f1c2b --- .../diagnostic/RollingFileHandlerTest.kt | 58 ++++++++++++++----- .../openapi/diagnostic/RollingFileHandler.kt | 19 ++++-- 2 files changed, 58 insertions(+), 19 deletions(-) diff --git a/platform/platform-tests/testSrc/com/intellij/openapi/diagnostic/RollingFileHandlerTest.kt b/platform/platform-tests/testSrc/com/intellij/openapi/diagnostic/RollingFileHandlerTest.kt index 19ea538442e9..c016e0e550e6 100644 --- a/platform/platform-tests/testSrc/com/intellij/openapi/diagnostic/RollingFileHandlerTest.kt +++ b/platform/platform-tests/testSrc/com/intellij/openapi/diagnostic/RollingFileHandlerTest.kt @@ -1,43 +1,71 @@ -// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. package com.intellij.openapi.diagnostic -import com.intellij.testFramework.TemporaryDirectory -import org.junit.Assert -import org.junit.Assert.assertEquals +import com.intellij.openapi.util.io.IoTestUtil +import com.intellij.openapi.util.io.NioFiles +import com.intellij.testFramework.rules.TempDirectory +import com.intellij.util.text.allOccurrencesOf +import org.junit.Assert.* import org.junit.Rule import org.junit.Test import java.nio.file.Files -import java.nio.file.Paths import java.util.logging.Formatter import java.util.logging.Level import java.util.logging.LogRecord class RollingFileHandlerTest { - @Rule @JvmField val tempDir = TemporaryDirectory() + @Rule @JvmField val tempDir = TempDirectory() + + private val msgOnlyFormatter = object : Formatter() { + override fun format(record: LogRecord): String = record.message + } @Test fun testRollingHandler() { - val logPath = tempDir.newPath("RollingFileHandlerTest.log") - val handler = RollingFileHandler(logPath, 100, 10, false) - handler.formatter = object : Formatter() { - override fun format(record: LogRecord): String = record.message - } + val logName = "RollingFileHandlerTest.log" + val logFile = tempDir.newFile(logName).toPath() + val handler = RollingFileHandler(logFile, 100, 10, false) + handler.formatter = msgOnlyFormatter val message1 = "a".repeat(80) handler.publish(LogRecord(Level.INFO, message1)) - assertEquals(message1, Files.readString(logPath)) + assertEquals(message1, Files.readString(logFile)) val message2 = "b".repeat(80) handler.publish(LogRecord(Level.INFO, message2)) - val logPath1 = Paths.get(logPath.toString().replace(".log", ".1.log")) + val logPath1 = logFile.resolveSibling(logName.replace(".log", ".1.log")) assertEquals(message1 + message2, Files.readString(logPath1)) val message3 = "c".repeat(80) handler.publish(LogRecord(Level.INFO, message3)) - assertEquals(message3, Files.readString(logPath)) + assertEquals(message3, Files.readString(logFile)) val message4 = "d".repeat(80) handler.publish(LogRecord(Level.INFO, message4)) - val logPath2 = Paths.get(logPath.toString().replace(".log", ".2.log")) + val logPath2 = logFile.resolveSibling(logName.replace(".log", ".2.log")) assertEquals(message3 + message4, Files.readString(logPath1)) assertEquals(message1 + message2, Files.readString(logPath2)) } + + @Test fun noWriteAmplificationOnFailedRotate() { + IoTestUtil.assumeUnix() + val logDir = tempDir.newDirectoryPath() + try { + val logName = "RollingFileHandlerTest.log" + val logFile = logDir.resolve(logName) + val handler = RollingFileHandler(logFile, 100, 2, false) + handler.formatter = msgOnlyFormatter + NioFiles.setReadOnly(logDir, true) + handler.publish(LogRecord(Level.INFO, "a".repeat(100))) + handler.publish(LogRecord(Level.INFO, "b".repeat(100))) + handler.publish(LogRecord(Level.INFO, "c".repeat(100))) + handler.publish(LogRecord(Level.INFO, "d".repeat(100))) + assertTrue(Files.exists(logFile)) + assertFalse(Files.exists(logFile.resolveSibling(logName.replace(".log", ".1.log")))) + handler.flush() + val content = Files.readString(logFile) + assertEquals(1, content.allOccurrencesOf("Log rotate failed: ").count()) + } + finally { + NioFiles.setReadOnly(logDir, false) + } + } } diff --git a/platform/util/src/com/intellij/openapi/diagnostic/RollingFileHandler.kt b/platform/util/src/com/intellij/openapi/diagnostic/RollingFileHandler.kt index a49d118bce00..a01264e3b99f 100644 --- a/platform/util/src/com/intellij/openapi/diagnostic/RollingFileHandler.kt +++ b/platform/util/src/com/intellij/openapi/diagnostic/RollingFileHandler.kt @@ -1,4 +1,4 @@ -// Copyright 2000-2022 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. +// Copyright 2000-2023 JetBrains s.r.o. and contributors. Use of this source code is governed by the Apache 2.0 license. package com.intellij.openapi.diagnostic import java.io.BufferedOutputStream @@ -18,6 +18,7 @@ class RollingFileHandler @JvmOverloads constructor( private val onRotate: Runnable? = null ) : StreamHandler() { @Volatile private lateinit var meter: MeteredOutputStream + private var rotateFailed: Boolean = false private class MeteredOutputStream(private val delegate: OutputStream, @Volatile var written: Long) : OutputStream() { override fun write(b: Int) { @@ -78,8 +79,7 @@ class RollingFileHandler @JvmOverloads constructor( } } catch (e: IOException) { - // rotate failed, keep writing to existing log - super.publish(LogRecord(Level.SEVERE, "Log rotate failed: ${e.message}").also { it.thrown = e }) + logRotateFailed(e) return } @@ -95,7 +95,18 @@ class RollingFileHandler @JvmOverloads constructor( open(false) - if (e != null) { + if (e == null) { + rotateFailed = false + } + else { + logRotateFailed(e) + } + } + + private fun logRotateFailed(e: IOException) { + if (!rotateFailed) { + // rotate failed, keep writing to existing log + rotateFailed = true super.publish(LogRecord(Level.SEVERE, "Log rotate failed: ${e.message}").also { it.thrown = e }) } }