[JENKINS-68417] Revert LogRecord tricks (#6643)

Co-authored-by: Basil Crow <[email protected]>
jenkinsci · Jul 6, 2022 · 75b4b31 · 75b4b31
1 parent 9dd2c74
commit 75b4b31
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 41 deletions.
diff --git a/core/src/main/java/hudson/util/RingBufferLogHandler.java b/core/src/main/java/hudson/util/RingBufferLogHandler.java
@@ -24,42 +24,22 @@
 
 package hudson.util;
 
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
-import java.lang.ref.SoftReference;
 import java.util.AbstractList;
 import java.util.List;
-import java.util.Objects;
 import java.util.logging.Handler;
-import java.util.logging.Level;
 import java.util.logging.LogRecord;
 
 /**
  * Log {@link Handler} that stores the log records into a ring buffer.
  *
  * @author Kohsuke Kawaguchi
  */
-@SuppressFBWarnings(
-        value = "RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT",
-        justification = "to guard against potential future compiler optimizations")
 public class RingBufferLogHandler extends Handler {
 
     private static final int DEFAULT_RING_BUFFER_SIZE = Integer.getInteger(RingBufferLogHandler.class.getName() + ".defaultSize", 256);
 
-    private static final class LogRecordRef extends SoftReference<LogRecord> {
-        LogRecordRef(LogRecord referent) {
-            super(referent);
-        }
-    }
-
-    static {
-        // Preload the LogRecordRef class to avoid an ABBA deadlock between agent class loading and logging.
-        LogRecord r = new LogRecord(Level.INFO, "<preloading>");
-        // We call Objects.hash() to guard against potential future compiler optimizations.
-        Objects.hash(new LogRecordRef(r).get());
-    }
-
     private int start = 0;
-    private final LogRecordRef[] records;
+    private final LogRecord[] records;
     private int size;
 
     /**
@@ -73,7 +53,7 @@ public RingBufferLogHandler() {
     }
 
     public RingBufferLogHandler(int ringSize) {
-        records = new LogRecordRef[ringSize];
+        records = new LogRecord[ringSize];
     }
 
     /**
@@ -86,13 +66,18 @@ public static int getDefaultRingBufferSize() {
     }
 
     @Override
-    public synchronized void publish(LogRecord record) {
-        int len = records.length;
-        records[(start + size) % len] = new LogRecordRef(record);
-        if (size == len) {
-            start = (start + 1) % len;
-        } else {
-            size++;
+    public void publish(LogRecord record) {
+        if (record == null) {
+            return;
+        }
+        synchronized (this) {
+            int len = records.length;
+            records[(start + size) % len] = record;
+            if (size == len) {
+                start = (start + 1) % len;
+            } else {
+                size++;
+            }
         }
     }
 
@@ -114,9 +99,7 @@ public List<LogRecord> getView() {
             public LogRecord get(int index) {
                 // flip the order
                 synchronized (RingBufferLogHandler.this) {
-                    LogRecord r = records[(start + (size - (index + 1))) % records.length].get();
-                    // We cannot just omit collected entries due to the List interface.
-                    return r != null ? r : new LogRecord(Level.OFF, "<discarded>");
+                    return records[(start + (size - (index + 1))) % records.length];
                 }
             }
 

diff --git a/test/src/test/java/hudson/util/RingBufferLogHandlerTest.java b/test/src/test/java/hudson/util/RingBufferLogHandlerTest.java
@@ -24,6 +24,8 @@
 
 package hudson.util;
 
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
 
@@ -33,10 +35,10 @@ public class RingBufferLogHandlerTest {
     @Issue("JENKINS-9120")
     public void tooMuchRecordsShouldNotCrashHandler() {
         final RingBufferLogHandler handler = new RingBufferLogHandler();
-
+        LogRecord lr = new LogRecord(Level.INFO, "xxx");
         for (long i = 0; i < (long) Integer.MAX_VALUE + 300; i++) {
             // throws ArrayIndexOutOfBoundsException after int-overflow
-            handler.publish(null);
+            handler.publish(lr);
         }
     }
 }
diff --git a/test/src/test/java/jenkins/model/JenkinsLogRecordsTest.java b/test/src/test/java/jenkins/model/JenkinsLogRecordsTest.java
@@ -9,7 +9,6 @@
 import hudson.util.VersionNumber;
 import java.lang.ref.WeakReference;
 import java.util.List;
-import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
@@ -37,14 +36,24 @@ private static void _logRecordsArePresentOnController(JenkinsRule r) throws Thro
             containsInRelativeOrder("Completed initialization", "Started initialization"));
         VersionNumber javaVersion = new VersionNumber(System.getProperty("java.specification.version"));
         if (javaVersion.isOlderThan(new VersionNumber("9")) || javaVersion.isNewerThanOrEqualTo(new VersionNumber("17"))) { // TODO https://github.com/jenkinsci/jenkins-test-harness/issues/359
-            LogRecord lr = new LogRecord(Level.INFO, "collect me");
-            Logger.getLogger(Jenkins.class.getName()).log(lr);
-            WeakReference<LogRecord> ref = new WeakReference<>(lr);
-            lr = null;
+            Object x = new Object() {
+                @Override
+                public String toString() {
+                    return "collect me";
+                }
+            };
+            use(x);
+            WeakReference<Object> ref = new WeakReference<>(x);
+            x = null;
             MemoryAssert.assertGC(ref, true);
-            assertThat("Records collected",
+            assertThat("Record parameters formatted before collection",
                 logRecords.stream().map(LogRecord::getMessage).collect(Collectors.toList()),
-                hasItem("<discarded>"));
+                hasItem("formatting collect me"));
         }
     }
+
+    private static void use(Object x) {
+        Logger.getLogger(Jenkins.class.getName()).info(() -> "formatting " + x);
+    }
+
 }