Merge pull request #663 from jglick/BoundObjectTable-JENKINS-16341

[JENKINS-16341] Automatically expire entries from `BoundObjectTable`

Author: jglick

core/src/main/java/org/kohsuke/stapler/bind/BoundObjectTable.java

@@ -23,22 +23,19 @@
 
 package org.kohsuke.stapler.bind;
 
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import jakarta.servlet.ServletException;
 import jakarta.servlet.http.HttpSession;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.Serializable;
-import java.lang.ref.WeakReference;
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.kohsuke.stapler.Ancestor;
-import org.kohsuke.stapler.HttpResponse;
-import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.QueryParameter;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerFallback;
@@ -149,22 +146,11 @@ public Table getStaplerFallback() {
         return resolve(false);
     }
 
-    private Bound bind(Ref ref) {
-        return resolve(true).add(ref);
-    }
-
     /**
      * Binds an object temporarily and returns its URL.
      */
     public Bound bind(Object o) {
-        return bind(new StrongRef(o));
-    }
-
-    /**
-     * Binds an object temporarily and returns its URL.
-     */
-    public Bound bindWeak(Object o) {
-        return bind(new WeakRef(o));
+        return resolve(true).add(new StrongRef(o));
     }
 
     /**
@@ -212,7 +198,6 @@ public Table getTable() {
      */
     public static class Table implements Serializable {
         private final Map<String, Ref> entries = new HashMap<>();
-        private boolean logging;
 
         private synchronized Bound add(Ref ref) {
             final Object target = ref.get();
@@ -225,11 +210,20 @@ private synchronized Bound add(Ref ref) {
                 return new WellKnownObjectHandle(url, w);
             }
 
+            // Before adding a new entry, clean up any old ones:
+            var it = entries.entrySet().iterator();
+            while (it.hasNext()) {
+                var entry = it.next();
+                if (!entry.getValue().valid()) {
+                    LOGGER.fine(() -> "removing stale " + entry.getKey() + ": "
+                            + entry.getValue().get());
+                    it.remove();
+                }
+            }
+
             final String id = UUID.randomUUID().toString();
             entries.put(id, ref);
-            if (logging) {
-                LOGGER.info(String.format("%s binding %s for %s", toString(), target, id));
-            }
+            LOGGER.fine(() -> String.format("%s binding %s for %s", toString(), target, id));
 
             return new Bound() {
                 @Override
@@ -259,38 +253,22 @@ public Object getDynamic(String id) {
             return resolve(id);
         }
 
-        private synchronized Ref release(String id) {
-            return entries.remove(id);
+        /**
+         * Releases a particular id from the table.
+         * Normally would be called via {@link #releaseMe} which infers the id from the current request.
+         */
+        public void release(String id) {
+            var ref = entries.remove(id);
+            LOGGER.fine(() -> "releasing " + id + ": " + (ref != null ? ref.get() : null));
         }
 
         private synchronized Object resolve(String id) {
             Ref e = entries.get(id);
             if (e == null) {
-                if (logging) {
-                    LOGGER.info(toString() + " doesn't have binding for " + id);
-                }
+                LOGGER.fine(() -> toString() + " doesn't have binding for " + id);
                 return null;
             }
-            Object v = e.get();
-            if (v == null) {
-                if (logging) {
-                    LOGGER.warning(toString() + " had binding for " + id + " but it got garbage collected");
-                }
-                entries.remove(id); // reference is already garbage collected.
-            }
-            return v;
-        }
-
-        @SuppressFBWarnings(
-                value = "IS2_INCONSISTENT_SYNC",
-                justification = "This usage does not create synchronization problems.")
-        public HttpResponse doEnableLogging() {
-            if (DEBUG_LOGGING) {
-                this.logging = true;
-                return HttpResponses.text("Logging enabled for this session: " + this + "\n");
-            } else {
-                return HttpResponses.forbidden();
-            }
+            return e.get();
         }
     }
 
@@ -332,54 +310,48 @@ public void generateResponse(StaplerRequest2 req, StaplerResponse2 rsp, Object n
      */
     interface Ref extends Serializable {
         Object get();
+
+        boolean valid();
     }
 
     private static class StrongRef implements Ref {
         private final Object o;
+        private final long expiration;
 
         StrongRef(Object o) {
             this.o = o;
+            expiration = System.currentTimeMillis() + EXPIRATION_TIME;
         }
 
         @Override
         public Object get() {
             return o;
         }
 
-        private Object writeReplace() {
-            if (o instanceof Serializable) {
-                return this;
-            } else {
-                LOGGER.fine(() -> "Refusing to serialize " + o);
-                return new StrongRef(null);
-            }
-        }
-    }
-
-    private static class WeakRef extends WeakReference implements Ref {
-        private WeakRef(Object referent) {
-            super(referent);
+        @Override
+        public boolean valid() {
+            return /* compatibility */ expiration == 0 || System.currentTimeMillis() < expiration;
         }
 
         private Object writeReplace() {
-            Object o = get();
             if (o instanceof Serializable) {
                 return this;
             } else {
                 LOGGER.fine(() -> "Refusing to serialize " + o);
-                return new WeakRef(null);
+                return new StrongRef(null);
             }
         }
     }
 
-    public static final String PREFIX = "/$stapler/bound/";
-    static final String SCRIPT_PREFIX = "/$stapler/bound/script";
-
     /**
-     * True to activate debug logging of session fragments.
+     * How long (in milliseconds) a {@link StrongRef} remains valid for creation if it is not retrieved.
      */
-    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
-    public static boolean DEBUG_LOGGING = Boolean.getBoolean(BoundObjectTable.class.getName() + ".debugLog");
+    private static final long EXPIRATION_TIME = Long.getLong(
+            BoundObjectTable.class.getName() + ".EXPIRATION_TIME",
+            Duration.ofDays(1).toMillis());
+
+    public static final String PREFIX = "/$stapler/bound/";
+    static final String SCRIPT_PREFIX = "/$stapler/bound/script";
 
     private static final Logger LOGGER = Logger.getLogger(BoundObjectTable.class.getName());
 }