Clean connector tracker after each access block to stop memory leaks (#9305)

Immediately clean connectors which the client side does not know about

Fixes #9303

server/src/main/java/com/vaadin/server/VaadinService.java

@@ -1410,6 +1410,17 @@ public abstract class VaadinService implements Serializable {
                 final long duration = (System.nanoTime() - (Long) request
                         .getAttribute(REQUEST_START_TIME_ATTRIBUTE)) / 1000000;
                 session.setLastRequestDuration(duration);
+
+                // Check that connector tracker is in a consistent state here to
+                // avoid doing it multiple times for a single request
+                for (UI ui : session.getUIs()) {
+                    try {
+                        ui.getConnectorTracker().ensureCleanedAndConsistent();
+                    } catch (AssertionError | Exception e) {
+                        getLogger().log(Level.SEVERE,
+                                "Error cleaning ConnectionTracker", e);
+                    }
+                }
             } finally {
                 session.unlock();
             }

server/src/main/java/com/vaadin/server/VaadinSession.java

@@ -895,9 +895,9 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
      *
      * @param createOnDemand
      *            <code>true</code> if a resource handler should be initialized
-     *            if there is no handler associated with this application.
-     *            </code>false</code> if </code>null</code> should be returned
-     *            if there is no registered handler.
+     *            if there is no handler associated with this application,
+     *            <code>false</code> if <code>null</code> should be returned if
+     *            there is no registered handler.
      * @return this session's global resource handler, or <code>null</code> if
      *         there is no handler and the createOnDemand parameter is
      *         <code>false</code>.
@@ -1013,6 +1013,14 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
                             CurrentInstance.restoreInstances(oldCurrent);
                         }
                     }
+                    try {
+                        ui.getConnectorTracker().cleanConnectorMap();
+                    } catch (Exception e) {
+                        getLogger().log(Level.SEVERE,
+                                "Exception while cleaning connector map for ui "
+                                        + ui.getUIId(),
+                                e);
+                    }
                 }
             }
         } finally {

server/src/main/java/com/vaadin/server/communication/UidlWriter.java

@@ -306,7 +306,6 @@ public class UidlWriter implements Serializable {
             writePerformanceData(ui, writer);
         } finally {
             uiConnectorTracker.setWritingResponse(false);
-            uiConnectorTracker.cleanConnectorMap();
         }
     }
 

server/src/main/java/com/vaadin/ui/ConnectorTracker.java

@@ -173,7 +173,15 @@ public class ConnectorTracker implements Serializable {
         }
 
         dirtyConnectors.remove(connector);
-        if (unregisteredConnectors.add(connector)) {
+
+        if (!isClientSideInitialized(connector)) {
+            // Client side has never known about this connector so there is no
+            // point in tracking it
+            removeUnregisteredConnector(connector,
+                    uI.getSession().getGlobalResourceHandler(false));
+        } else if (unregisteredConnectors.add(connector)) {
+            // Client side knows about the connector, track it for a while if it
+            // becomes reattached
             if (getLogger().isLoggable(Level.FINE)) {
                 getLogger().log(Level.FINE, "Unregistered {0} ({1})",
                         new Object[] { connector.getClass().getSimpleName(),
@@ -263,14 +271,24 @@ public class ConnectorTracker implements Serializable {
             removeUnregisteredConnectors();
         }
 
+        cleanStreamVariables();
+    }
+
+    /**
+     * Performs expensive checks to ensure that the connector tracker is cleaned
+     * properly and in a consistent state.
+     * <p>
+     * This should only be called by the framework.
+     *
+     * @since
+     */
+    public void ensureCleanedAndConsistent() {
         // Do this expensive check only with assertions enabled
         assert isHierarchyComplete() : "The connector hierarchy is corrupted. "
                 + "Check for missing calls to super.setParent(), super.attach() and super.detach() "
                 + "and that all custom component containers call child.setParent(this) when a child is added and child.setParent(null) when the child is no longer used. "
                 + "See previous log messages for details.";
 
-        // remove detached components from paintableIdMap so they
-        // can be GC'ed
         Iterator<ClientConnector> iterator = connectorIdToConnector.values()
                 .iterator();
         GlobalResourceHandler globalResourceHandler = uI.getSession()
@@ -283,14 +301,11 @@ public class ConnectorTracker implements Serializable {
                 // remove it from the map. If it is re-attached to the
                 // application at some point it will be re-added through
                 // registerConnector(connector)
-
                 // This code should never be called as cleanup should take place
                 // in detach()
-
                 getLogger().log(Level.WARNING,
                         "cleanConnectorMap unregistered connector {0}. This should have been done when the connector was detached.",
                         getConnectorAndParentInfo(connector));
-
                 if (globalResourceHandler != null) {
                     globalResourceHandler.unregisterConnector(connector);
                 }
@@ -302,11 +317,9 @@ public class ConnectorTracker implements Serializable {
                             .isConnectorVisibleToClient(connector)) {
                 uninitializedConnectors.add(connector);
                 diffStates.remove(connector);
-
                 assert isRemovalSentToClient(connector) : "Connector "
                         + connector + " (id = " + connector.getConnectorId()
-                        + ") is no longer visible to the client, but no corresponding hierarchy change is being sent.";
-
+                        + ") is no longer visible to the client, but no corresponding hierarchy change was sent.";
                 if (getLogger().isLoggable(Level.FINE)) {
                     getLogger().log(Level.FINE,
                             "cleanConnectorMap removed state for {0} as it is not visible",
@@ -314,8 +327,6 @@ public class ConnectorTracker implements Serializable {
                 }
             }
         }
-
-        cleanStreamVariables();
     }
 
     private boolean isRemovalSentToClient(ClientConnector connector) {
@@ -398,24 +409,48 @@ public class ConnectorTracker implements Serializable {
         return null;
     }
 
+    /**
+     * Removes all references and information about connectors marked as
+     * unregistered.
+     *
+     */
     private void removeUnregisteredConnectors() {
         GlobalResourceHandler globalResourceHandler = uI.getSession()
                 .getGlobalResourceHandler(false);
 
         for (ClientConnector connector : unregisteredConnectors) {
-            ClientConnector removedConnector = connectorIdToConnector
-                    .remove(connector.getConnectorId());
-            assert removedConnector == connector;
-
-            if (globalResourceHandler != null) {
-                globalResourceHandler.unregisterConnector(connector);
-            }
-            uninitializedConnectors.remove(connector);
-            diffStates.remove(connector);
+            removeUnregisteredConnector(connector, globalResourceHandler);
         }
         unregisteredConnectors.clear();
     }
 
+    /**
+     * Removes all references and information about the given connector, which
+     * must not be registered.
+     *
+     * @param connector
+     * @param globalResourceHandler
+     */
+    private void removeUnregisteredConnector(ClientConnector connector,
+            GlobalResourceHandler globalResourceHandler) {
+        ClientConnector removedConnector = connectorIdToConnector
+                .remove(connector.getConnectorId());
+        assert removedConnector == connector;
+
+        if (globalResourceHandler != null) {
+            globalResourceHandler.unregisterConnector(connector);
+        }
+        uninitializedConnectors.remove(connector);
+        diffStates.remove(connector);
+    }
+
+    /**
+     * Checks that the connector hierarchy is consistent.
+     *
+     * @return <code>true</code> if the hierarchy is consistent,
+     *         <code>false</code> otherwise
+     * @since
+     */
     private boolean isHierarchyComplete() {
         boolean noErrors = true;
 

server/src/test/java/com/vaadin/ui/UITest.java

@@ -1,5 +1,6 @@
 package com.vaadin.ui;
 
+import java.lang.ref.WeakReference;
 import java.util.Properties;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.CountDownLatch;
@@ -10,6 +11,7 @@ import javax.servlet.ServletConfig;
 
 import org.junit.Assert;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 import com.vaadin.server.DefaultDeploymentConfiguration;
 import com.vaadin.server.MockServletConfig;
@@ -20,6 +22,7 @@ import com.vaadin.server.VaadinServletService;
 import com.vaadin.server.VaadinSession;
 import com.vaadin.server.communication.PushConnection;
 import com.vaadin.shared.communication.PushMode;
+import com.vaadin.util.CurrentInstanceTest;
 
 public class UITest {
 
@@ -152,4 +155,57 @@ public class UITest {
         Assert.assertNull(ui.getPushConnection());
 
     }
+
+    @Test
+    public void connectorTrackerMemoryLeak() throws Exception {
+        final UI ui = new UI() {
+
+            @Override
+            protected void init(VaadinRequest request) {
+            }
+
+        };
+        ServletConfig servletConfig = new MockServletConfig();
+        VaadinServlet servlet = new VaadinServlet();
+        servlet.init(servletConfig);
+
+        DefaultDeploymentConfiguration deploymentConfiguration = new DefaultDeploymentConfiguration(
+                UI.class, new Properties());
+
+        VaadinServletService service = new VaadinServletService(servlet,
+                deploymentConfiguration);
+        MockVaadinSession session = new MockVaadinSession(service);
+        session.lock();
+        ui.setSession(session);
+        ui.doInit(Mockito.mock(VaadinRequest.class), 1, "foo");
+        session.addUI(ui);
+        ui.setContent(createContent());
+        WeakReference<Component> contentSentToClient = new WeakReference<>(
+                ui.getContent());
+        ui.getConnectorTracker()
+                .markClientSideInitialized(contentSentToClient.get());
+        session.unlock();
+
+        session.lock();
+        ui.setContent(createContent());
+        WeakReference<Component> contentOnlyOnServer = new WeakReference<>(
+                ui.getContent());
+        ui.setContent(createContent());
+
+        CurrentInstanceTest.waitUntilGarbageCollected(contentOnlyOnServer);
+        // Should not clean references for connectors available in the browser
+        // until the session is unlocked and we know if it has been moved
+        Assert.assertNotNull(contentSentToClient.get());
+        session.unlock();
+        CurrentInstanceTest.waitUntilGarbageCollected(contentSentToClient);
+    }
+
+    private Component createContent() {
+        VerticalLayout vl = new VerticalLayout();
+        vl.addComponent(new Button("foo"));
+        vl.addComponent(new Button("bar"));
+        vl.addComponent(new Button("baz"));
+        return vl;
+    }
+
 }

server/src/test/java/com/vaadin/util/CurrentInstanceTest.java

@@ -159,7 +159,7 @@ public class CurrentInstanceTest {
         Assert.assertNull(VaadinSession.getCurrent());
     }
 
-    private static void waitUntilGarbageCollected(WeakReference<?> ref)
+    public static void waitUntilGarbageCollected(WeakReference<?> ref)
             throws InterruptedException {
         for (int i = 0; i < 50; i++) {
             System.gc();

uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java

@@ -15,8 +15,16 @@
  */
 package com.vaadin.tests.application;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.logging.Handler;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+
 import com.vaadin.server.VaadinRequest;
-import com.vaadin.tests.components.AbstractReindeerTestUI;
+import com.vaadin.server.VaadinService;
+import com.vaadin.tests.components.AbstractReindeerTestUIWithLog;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.Component;
@@ -24,13 +32,15 @@ import com.vaadin.ui.CssLayout;
 import com.vaadin.ui.Label;
 import com.vaadin.ui.SelectiveRenderer;
 
-public class MissingHierarchyDetection extends AbstractReindeerTestUI {
+public class MissingHierarchyDetection extends AbstractReindeerTestUIWithLog {
 
     private boolean isChildRendered = true;
     private BrokenCssLayout brokenLayout = new BrokenCssLayout();
 
     private CssLayout normalLayout = new CssLayout(
             new Label("Normal layout child"));
+    private List<LogRecord> pendingErrors = Collections
+            .synchronizedList(new ArrayList<>());
 
     public class BrokenCssLayout extends CssLayout
             implements SelectiveRenderer {
@@ -49,6 +59,29 @@ public class MissingHierarchyDetection extends AbstractReindeerTestUI {
 
     @Override
     protected void setup(VaadinRequest request) {
+        // Catch log messages so we can see if there is an error
+        Logger vaadinServiceLogger = Logger
+                .getLogger(VaadinService.class.getName());
+        vaadinServiceLogger.addHandler(new Handler() {
+
+            @Override
+            public void publish(LogRecord record) {
+                if (record.getThrown() instanceof AssertionError) {
+                    pendingErrors.add(record);
+                    vaadinServiceLogger.removeHandler(this);
+                }
+            }
+
+            @Override
+            public void flush() {
+
+            }
+
+            @Override
+            public void close() throws SecurityException {
+
+            }
+        });
         addComponent(brokenLayout);
         addComponent(normalLayout);
         addComponent(new Button("Toggle properly", new Button.ClickListener() {
@@ -64,6 +97,16 @@ public class MissingHierarchyDetection extends AbstractReindeerTestUI {
                         toggle(false);
                     }
                 }));
+        addComponent(new Button("Check for errors", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                if (!pendingErrors.isEmpty()) {
+                    log(pendingErrors.remove(0).getThrown().getMessage());
+                } else {
+                    log("No errors");
+                }
+            }
+        }));
     }
 
     private void toggle(boolean properly) {

uitest/src/main/java/com/vaadin/tests/components/ui/ConnectorTrackerMemoryLeakUI.java

@@ -0,0 +1,78 @@
+package com.vaadin.tests.components.ui;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.CssLayout;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.UI;
+import com.vaadin.ui.VerticalLayout;
+import com.vaadin.ui.Window;
+
+public class ConnectorTrackerMemoryLeakUI extends UI {
+
+    public static final String BUTTON_CAPTION = "Kill!";
+    public static final String LABEL_STOPPED = "Still alive!";
+    private CssLayout panel = new CssLayout();
+    private List<String> items = new ArrayList<>(200);
+    private VerticalLayout layout = new VerticalLayout();
+
+    @Override
+    protected void init(VaadinRequest vaadinRequest) {
+
+        Button button = new Button(BUTTON_CAPTION);
+        button.addClickListener(e -> {
+            gc();
+            long memory = Runtime.getRuntime().totalMemory();
+            System.out.println("Before: " + memory);
+            // To simulate 200 concurrent session we do this 200 times
+            for (int i = 0; i < 200; i++) {
+
+                // Clear items
+                items.clear();
+                for (int j = 1; j <= 200; j++) {
+
+                    // Add one item and update the panel with those
+                    items.add("Item #" + j);
+                    updatePanel(panel, items);
+                }
+            }
+
+            // We made it this far. Good for us.
+            Label labelStop = new Label(LABEL_STOPPED);
+            layout.addComponent(labelStop);
+            gc();
+            long delta = Runtime.getRuntime().totalMemory() - memory;
+            memory = memory + delta;
+            System.out.println(" After: " + memory + " (+" + delta + ")");
+        });
+
+        layout.addComponents(button, panel);
+        setContent(layout);
+    }
+
+    private void gc() {
+        // Sometimes the VM needs a couple of "suggestions" to actually
+        // perform gc. There is no automated test for this UI so tweak if
+        // needed.
+        for (int i = 0; i < 3; i++) {
+            Runtime.getRuntime().gc();
+        }
+
+        try {
+            Thread.sleep(500);
+        } catch (InterruptedException e1) {
+        }
+    }
+
+    private static void updatePanel(CssLayout panel, List<String> items) {
+        panel.removeAllComponents();
+        items.forEach(i -> panel.addComponent(new Button(i, e -> {
+            Window w = new Window();
+            UI.getCurrent().addWindow(w);
+        })));
+    }
+
+}

uitest/src/test/java/com/vaadin/tests/application/MissingHierarchyDetectionTest.java

@@ -41,10 +41,14 @@ public class MissingHierarchyDetectionTest extends SingleBrowserTest {
         assertNoSystemNotifications();
         Assert.assertTrue(isElementPresent(LabelElement.class));
 
-        ButtonElement toggleInproperly = $(ButtonElement.class)
+        ButtonElement toggleImproperly = $(ButtonElement.class)
                 .caption("Toggle improperly").first();
-        toggleInproperly.click();
+        toggleImproperly.click();
 
-        assertSystemNotification();
+        $(ButtonElement.class).caption("Check for errors").first().click();
+        Assert.assertTrue(
+                "No error was logged for the missing hierarchy change event",
+                getLogRow(0).contains(
+                        "is no longer visible to the client, but no corresponding hierarchy change was sent."));
     }
 }