diff options
9 files changed, 264 insertions, 30 deletions
diff --git a/server/src/main/java/com/vaadin/server/VaadinService.java b/server/src/main/java/com/vaadin/server/VaadinService.java index 3b56457c92..e9973e132f 100644 --- a/server/src/main/java/com/vaadin/server/VaadinService.java +++ b/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(); } diff --git a/server/src/main/java/com/vaadin/server/VaadinSession.java b/server/src/main/java/com/vaadin/server/VaadinSession.java index c1995516de..de5c192846 100644 --- a/server/src/main/java/com/vaadin/server/VaadinSession.java +++ b/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 { 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 7511ccd7ec..ee25a95f8a 100644 --- a/server/src/main/java/com/vaadin/server/communication/UidlWriter.java +++ b/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(); } } diff --git a/server/src/main/java/com/vaadin/ui/ConnectorTracker.java b/server/src/main/java/com/vaadin/ui/ConnectorTracker.java index 79afb4b66d..a4476ce9d8 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. + * <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; diff --git a/server/src/test/java/com/vaadin/ui/UITest.java b/server/src/test/java/com/vaadin/ui/UITest.java index 322bc81a04..8bc3c98470 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<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; + } + } diff --git a/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java b/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java index e2da8e0278..dd4cc3f543 100644 --- a/server/src/test/java/com/vaadin/util/CurrentInstanceTest.java +++ b/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(); 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 8f35d69112..54c19c3353 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.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) { 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..fdcfe24776 --- /dev/null +++ b/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); + }))); + } + +} 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 2a97dbaaf6..80eb0735da 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.")); } } |