From 57d0b2fd4c87e3dfc64e33fc8a43e78c8394f31e Mon Sep 17 00:00:00 2001 From: Artur Date: Wed, 17 May 2017 13:06:47 +0300 Subject: [PATCH] Clean connector tracker after each access block to stop memory leaks (#9331) Immediately clean connectors which the client side does not know about Fixes #9303 --- .../java/com/vaadin/server/VaadinService.java | 14 +++ .../java/com/vaadin/server/VaadinSession.java | 14 ++- .../server/communication/UidlWriter.java | 1 - .../java/com/vaadin/ui/ConnectorTracker.java | 75 +++++++++++----- .../src/test/java/com/vaadin/ui/UITest.java | 56 ++++++++++++ .../com/vaadin/util/CurrentInstanceTest.java | 2 +- .../MissingHierarchyDetection.java | 47 +++++++++- .../ui/ConnectorTrackerMemoryLeakUI.java | 87 +++++++++++++++++++ .../MissingHierarchyDetectionTest.java | 10 ++- 9 files changed, 276 insertions(+), 30 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/components/ui/ConnectorTrackerMemoryLeakUI.java diff --git a/server/src/main/java/com/vaadin/server/VaadinService.java b/server/src/main/java/com/vaadin/server/VaadinService.java index e453332f02..75124aa6ef 100644 --- a/server/src/main/java/com/vaadin/server/VaadinService.java +++ b/server/src/main/java/com/vaadin/server/VaadinService.java @@ -1365,6 +1365,20 @@ 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 e) { + getLogger().log(Level.SEVERE, + "Error cleaning ConnectionTracker", e); + } catch (Exception e) { + getLogger().log(Level.SEVERE, + "Error cleaning ConnectionTracker", e); + } + } } finally { session.unlock(); } diff --git a/server/src/main/java/com/vaadin/server/VaadinSession.java b/server/src/main/java/com/vaadin/server/VaadinSession.java index b0a99c1de9..f4ebc06335 100644 --- a/server/src/main/java/com/vaadin/server/VaadinSession.java +++ b/server/src/main/java/com/vaadin/server/VaadinSession.java @@ -884,9 +884,9 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { * * @param createOnDemand * true if a resource handler should be initialized - * if there is no handler associated with this application. - * false if null should be returned - * if there is no registered handler. + * if there is no handler associated with this application, + * false if null should be returned if + * there is no registered handler. * @return this session's global resource handler, or null if * there is no handler and the createOnDemand parameter is * false. @@ -1002,6 +1002,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 { diff --git a/server/src/main/java/com/vaadin/server/communication/UidlWriter.java b/server/src/main/java/com/vaadin/server/communication/UidlWriter.java index c61060d95f..2a55b52214 100644 --- a/server/src/main/java/com/vaadin/server/communication/UidlWriter.java +++ b/server/src/main/java/com/vaadin/server/communication/UidlWriter.java @@ -331,7 +331,6 @@ public class UidlWriter implements Serializable { writePerformanceData(ui, writer); } finally { uiConnectorTracker.setWritingResponse(false); - uiConnectorTracker.cleanConnectorMap(); } } diff --git a/server/src/main/java/com/vaadin/ui/ConnectorTracker.java b/server/src/main/java/com/vaadin/ui/ConnectorTracker.java index c3b1cfe98f..043c1174fb 100644 --- a/server/src/main/java/com/vaadin/ui/ConnectorTracker.java +++ b/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. + *

+ * 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 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 true if the hierarchy is consistent, + * false otherwise + * @since + */ private boolean isHierarchyComplete() { boolean noErrors = true; diff --git a/server/src/test/java/com/vaadin/ui/UITest.java b/server/src/test/java/com/vaadin/ui/UITest.java index 322bc81a04..702e107fb2 100644 --- a/server/src/test/java/com/vaadin/ui/UITest.java +++ b/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 contentSentToClient = new WeakReference( + ui.getContent()); + ui.getConnectorTracker() + .markClientSideInitialized(contentSentToClient.get()); + session.unlock(); + + session.lock(); + ui.setContent(createContent()); + WeakReference 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; + } + } diff --git a/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java b/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java index f9cf2549ea..ef356407d6 100644 --- a/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java +++ b/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java @@ -234,7 +234,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(); diff --git a/uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java b/uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java index 9e463569c0..ff60b66f20 100644 --- a/uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java +++ b/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.AbstractTestUI; +import com.vaadin.tests.components.AbstractTestUIWithLog; +import com.vaadin.server.VaadinService; 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 AbstractTestUI { +public class MissingHierarchyDetection extends AbstractTestUIWithLog { private boolean isChildRendered = true; private BrokenCssLayout brokenLayout = new BrokenCssLayout(); private CssLayout normalLayout = new CssLayout( new Label("Normal layout child")); + private List pendingErrors = Collections + .synchronizedList(new ArrayList()); public class BrokenCssLayout extends CssLayout implements SelectiveRenderer { @@ -49,6 +59,29 @@ public class MissingHierarchyDetection extends AbstractTestUI { @Override protected void setup(VaadinRequest request) { + // Catch log messages so we can see if there is an error + final 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 AbstractTestUI { 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) { diff --git a/uitest/src/main/java/com/vaadin/tests/components/ui/ConnectorTrackerMemoryLeakUI.java b/uitest/src/main/java/com/vaadin/tests/components/ui/ConnectorTrackerMemoryLeakUI.java new file mode 100644 index 0000000000..a8f9d08d8d --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/ui/ConnectorTrackerMemoryLeakUI.java @@ -0,0 +1,87 @@ +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.Button.ClickEvent; +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 items = new ArrayList(200); + private VerticalLayout layout = new VerticalLayout(); + + @Override + protected void init(VaadinRequest vaadinRequest) { + + Button button = new Button(BUTTON_CAPTION); + button.addClickListener(new Button.ClickListener() { + @Override + public void buttonClick(ClickEvent event) { + 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 items) { + panel.removeAllComponents(); + for (String i : items) { + panel.addComponent(new Button(i, new Button.ClickListener() { + @Override + public void buttonClick(ClickEvent event) { + Window w = new Window(); + UI.getCurrent().addWindow(w); + } + })); + } + } + +} diff --git a/uitest/src/test/java/com/vaadin/tests/application/MissingHierarchyDetectionTest.java b/uitest/src/test/java/com/vaadin/tests/application/MissingHierarchyDetectionTest.java index ad44fe6f89..c047c9a12b 100644 --- a/uitest/src/test/java/com/vaadin/tests/application/MissingHierarchyDetectionTest.java +++ b/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.")); } } -- 2.39.5