[FIXED JENKINS-25081] Memory leak in BoundObjectTable when Progressiv…

…eRendering is used. (cherry picked from commit aac8c23) Conflicts: changelog.html
jenkinsci · Nov 7, 2014 · 3eefad8 · 3eefad8
1 parent 03a13fc
commit 3eefad8
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 10 deletions.
diff --git a/core/src/main/java/jenkins/util/ProgressiveRendering.java b/core/src/main/java/jenkins/util/ProgressiveRendering.java
@@ -35,6 +35,8 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
@@ -47,7 +49,10 @@
 import org.kohsuke.stapler.RequestImpl;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.TokenList;
+import org.kohsuke.stapler.bind.Bound;
+import org.kohsuke.stapler.bind.BoundObjectTable;
 import org.kohsuke.stapler.bind.JavaScriptMethod;
+import org.kohsuke.stapler.jelly.BindTag;
 
 /**
  * A helper thread which does some computation in the background and displays incremental results using JavaScript.
@@ -79,23 +84,36 @@ public abstract class ProgressiveRendering {
 
     private double status = 0;
     private long lastNewsTime;
+    private final SecurityContext securityContext;
+    private final RequestImpl request;
     /** just for logging */
-    private String uri;
+    private final String uri;
     private long start;
+    private BoundObjectTable.Table boundObjectTable;
+    /** Unfortunately we cannot get the {@link Bound} that was created for us; it is thrown out by {@link BindTag}. */
+    private String boundId;
 
     /** Constructor for subclasses. */
     protected ProgressiveRendering() {
+        securityContext = SecurityContextHolder.getContext();
+        request = createMockRequest();
+        uri = request.getRequestURI();
     }
 
     /**
      * For internal use.
      */
     @SuppressWarnings("RV_RETURN_VALUE_IGNORED_BAD_PRACTICE")
-    public final void start() {
-        final SecurityContext securityContext = SecurityContextHolder.getContext();
-        final RequestImpl request = createMockRequest();
-        uri = request.getRequestURI();
-        executorService().submit(new Runnable() {
+    @JavaScriptMethod public final void start() {
+        Ancestor ancestor = Stapler.getCurrentRequest().findAncestor(BoundObjectTable.class);
+        if (ancestor == null) {
+            throw new IllegalStateException("no BoundObjectTable");
+        }
+        boundObjectTable = ((BoundObjectTable) ancestor.getObject()).getTable();
+        boundId = ancestor.getNextToken(0);
+        LOG.log(Level.FINE, "starting rendering {0} at {1}", new Object[] {uri, boundId});
+        final ExecutorService executorService = executorService();
+        executorService.submit(new Runnable() {
             @Override public void run() {
                 lastNewsTime = start = System.currentTimeMillis();
                 setCurrentRequest(request);
@@ -114,10 +132,29 @@ public final void start() {
                     setCurrentRequest(null);
                     LOG.log(Level.FINE, "{0} finished in {1}msec with status {2}", new Object[] {uri, System.currentTimeMillis() - start, status});
                 }
+                if (executorService instanceof ScheduledExecutorService) {
+                    ((ScheduledExecutorService) executorService).schedule(new Runnable() {
+                        @Override public void run() {
+                            LOG.log(Level.FINE, "some time has elapsed since {0} finished, so releasing", boundId);
+                            release();
+                        }
+                    }, timeout() /* add some grace period for browser/network overhead */ * 2, TimeUnit.MILLISECONDS);
+                }
             }
         });
     }
 
+    /** {@link BoundObjectTable#releaseMe} just cannot work the way we need it to. */
+    private void release() {
+        try {
+            Method release = BoundObjectTable.Table.class.getDeclaredMethod("release", String.class);
+            release.setAccessible(true);
+            release.invoke(boundObjectTable, boundId);
+        } catch (Exception x) {
+            LOG.log(Level.WARNING, "failed to unbind " + boundId, x);
+        }
+    }
+
     /**
      * Copies important fields from the current HTTP request and makes them available during {@link #compute}.
      * This is necessary because some model methods such as {@link AbstractItem#getUrl} behave differently when called from a request.
@@ -142,7 +179,7 @@ private static RequestImpl createMockRequest() {
             }
         }
         List/*<AncestorImpl>*/ ancestors = currentRequest.ancestors;
-        LOG.log(Level.FINE, "mocking ancestors {0} using {1}", new Object[] {ancestors, getters});
+        LOG.log(Level.FINER, "mocking ancestors {0} using {1}", new Object[] {ancestors, getters});
         TokenList tokens = currentRequest.tokens;
         return new RequestImpl(Stapler.getCurrent(), (HttpServletRequest) Proxy.newProxyInstance(ProgressiveRendering.class.getClassLoader(), new Class<?>[] {HttpServletRequest.class}, new InvocationHandler() {
             @Override public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
@@ -242,7 +279,12 @@ protected final boolean canceled() {
             LOG.log(Level.WARNING, "failed to update " + uri, x);
             status = ERROR;
         }
-        r.put("status", status == 1 ? "done" : status == CANCELED ? "canceled" : status == ERROR ? "error" : status);
+        Object statusJSON = status == 1 ? "done" : status == CANCELED ? "canceled" : status == ERROR ? "error" : status;
+        r.put("status", statusJSON);
+        if (statusJSON instanceof String) { // somehow completed
+            LOG.log(Level.FINE, "finished in news so releasing {0}", boundId);
+            release();
+        }
         lastNewsTime = System.currentTimeMillis();
         LOG.log(Level.FINER, "news from {0}", uri);
         return r;

diff --git a/core/src/main/resources/lib/layout/progressiveRendering.jelly b/core/src/main/resources/lib/layout/progressiveRendering.jelly
@@ -37,7 +37,6 @@ THE SOFTWARE.
     <st:adjunct includes="lib.layout.progressiveRendering.progressiveRendering"/>
     <j:set var="id" value="${h.generateId()}"/>
     <t:progressBar id="${id}" pos="0" tooltip="${tooltip ?: '%progressMessage'}"/>
-    <j:invoke method="start" on="${handler}"/>
     <st:bind var="proxy" value="${handler}" />
     <script>progressivelyRender(proxy, ${callback}, '${id}');</script>
 </j:jelly>
diff --git a/core/src/main/resources/lib/layout/progressiveRendering/progressiveRendering.js b/core/src/main/resources/lib/layout/progressiveRendering/progressiveRendering.js
@@ -47,5 +47,7 @@ function progressivelyRender(handler, callback, statusId) {
             handler.news(checkNews);
         }, timeout);
     }
-    checkNewsLater(0);
+    handler.start(function(response) {
+        checkNewsLater(0);
+    });
 }