From 3766ab61c90885c8657186c4b033667dc2fa25db Mon Sep 17 00:00:00 2001 From: Artur Date: Tue, 1 Aug 2017 10:56:41 +0300 Subject: [PATCH] Do full connector tracker cleanup when the session lock is released (#9707) (#9730) As there is no "request end" call after invoking UI.access() from a background thread, the connector map was not earlier properly cleaned afterwards. If you toggled visibility of a component from the background thread, the tracker state became inconsistent. If this becomes a performance problem, it could probably be optimized to that cleanup is done in request end and only at the end of access if not inside a request. Backported from master Fixes #9693 --- .../java/com/vaadin/server/VaadinService.java | 14 ---- .../java/com/vaadin/server/VaadinSession.java | 5 ++ .../java/com/vaadin/ui/ConnectorTracker.java | 14 +--- .../MissingHierarchyDetection.java | 9 ++- .../push/PushToggleComponentVisibility.java | 73 +++++++++++++++++++ .../PushToggleComponentVisibilityTest.java | 49 +++++++++++++ 6 files changed, 136 insertions(+), 28 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/push/PushToggleComponentVisibility.java create mode 100644 uitest/src/test/java/com/vaadin/tests/push/PushToggleComponentVisibilityTest.java diff --git a/server/src/main/java/com/vaadin/server/VaadinService.java b/server/src/main/java/com/vaadin/server/VaadinService.java index 75124aa6ef..e453332f02 100644 --- a/server/src/main/java/com/vaadin/server/VaadinService.java +++ b/server/src/main/java/com/vaadin/server/VaadinService.java @@ -1365,20 +1365,6 @@ 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 f4ebc06335..7014a5f1f5 100644 --- a/server/src/main/java/com/vaadin/server/VaadinSession.java +++ b/server/src/main/java/com/vaadin/server/VaadinSession.java @@ -1009,6 +1009,11 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable { "Exception while cleaning connector map for ui " + ui.getUIId(), e); + } catch (AssertionError e) { + getLogger().log(Level.SEVERE, + "Exception while cleaning connector map for ui " + + ui.getUIId(), + e); } } } diff --git a/server/src/main/java/com/vaadin/ui/ConnectorTracker.java b/server/src/main/java/com/vaadin/ui/ConnectorTracker.java index 043c1174fb..d2b9b65764 100644 --- a/server/src/main/java/com/vaadin/ui/ConnectorTracker.java +++ b/server/src/main/java/com/vaadin/ui/ConnectorTracker.java @@ -272,17 +272,7 @@ public class ConnectorTracker implements Serializable { } 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() " @@ -315,6 +305,10 @@ public class ConnectorTracker implements Serializable { } else if (!uninitializedConnectors.contains(connector) && !LegacyCommunicationManager .isConnectorVisibleToClient(connector)) { + // Connector was visible to the client but is no longer (e.g. + // setVisible(false) has been called or SelectiveRenderer tells + // it's no longer shown) -> make sure that the full state is + // sent again when/if made visible uninitializedConnectors.add(connector); diffStates.remove(connector); assert isRemovalSentToClient(connector) : "Connector " 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 ff60b66f20..51abf037ff 100644 --- a/uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java +++ b/uitest/src/main/java/com/vaadin/tests/application/MissingHierarchyDetection.java @@ -25,6 +25,7 @@ import java.util.logging.Logger; import com.vaadin.server.VaadinRequest; import com.vaadin.tests.components.AbstractTestUIWithLog; import com.vaadin.server.VaadinService; +import com.vaadin.server.VaadinSession; import com.vaadin.ui.Button; import com.vaadin.ui.Button.ClickEvent; import com.vaadin.ui.Component; @@ -60,15 +61,15 @@ public class MissingHierarchyDetection extends AbstractTestUIWithLog { @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() { + final Logger vaadinSessionLogger = Logger + .getLogger(VaadinSession.class.getName()); + vaadinSessionLogger.addHandler(new Handler() { @Override public void publish(LogRecord record) { if (record.getThrown() instanceof AssertionError) { pendingErrors.add(record); - vaadinServiceLogger.removeHandler(this); + vaadinSessionLogger.removeHandler(this); } } diff --git a/uitest/src/main/java/com/vaadin/tests/push/PushToggleComponentVisibility.java b/uitest/src/main/java/com/vaadin/tests/push/PushToggleComponentVisibility.java new file mode 100644 index 0000000000..fe8da7b946 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/push/PushToggleComponentVisibility.java @@ -0,0 +1,73 @@ +/* + * Copyright 2000-2016 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.push; + +import com.vaadin.annotations.Push; +import com.vaadin.annotations.Widgetset; +import com.vaadin.server.VaadinRequest; +import com.vaadin.ui.Button; +import com.vaadin.ui.Button.ClickEvent; +import com.vaadin.ui.Button.ClickListener; +import com.vaadin.ui.Label; +import com.vaadin.ui.UI; +import com.vaadin.ui.VerticalLayout; + +@Push +@Widgetset("com.vaadin.DefaultWidgetSet") +public class PushToggleComponentVisibility extends UI { + + @Override + protected void init(VaadinRequest request) { + VerticalLayout mainLayout = new VerticalLayout(); + setContent(mainLayout); + + final Label label = new Label("Please wait"); + label.setId("label"); + label.setVisible(false); + mainLayout.addComponent(label); + + final Button button = new Button("Hide me for 3 seconds"); + button.setId("hide"); + button.addClickListener(new ClickListener() { + @Override + public void buttonClick(ClickEvent event1) { + button.setVisible(false); + label.setVisible(true); + + new Thread(new Runnable() { + @Override + public void run() { + try { + Thread.sleep(3000); + } catch (InterruptedException e) { + e.printStackTrace(); + } + button.getUI().access(new Runnable() { + @Override + public void run() { + button.setVisible(true); + label.setVisible(false); + button.getUI().push(); + } + }); + } + }).start(); + } + }); + mainLayout.addComponent(button); + } + +} diff --git a/uitest/src/test/java/com/vaadin/tests/push/PushToggleComponentVisibilityTest.java b/uitest/src/test/java/com/vaadin/tests/push/PushToggleComponentVisibilityTest.java new file mode 100644 index 0000000000..011fa55e83 --- /dev/null +++ b/uitest/src/test/java/com/vaadin/tests/push/PushToggleComponentVisibilityTest.java @@ -0,0 +1,49 @@ +/* + * Copyright 2000-2016 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ +package com.vaadin.tests.push; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.support.ui.ExpectedCondition; + +import com.vaadin.testbench.elements.ButtonElement; +import com.vaadin.testbench.elements.LabelElement; +import com.vaadin.tests.tb3.SingleBrowserTest; + +public class PushToggleComponentVisibilityTest extends SingleBrowserTest { + + private static final String HIDE = "hide"; + + @Test + public void ensureComponentVisible() { + openTestURL(); + + $(ButtonElement.class).id(HIDE).click(); + Assert.assertEquals("Please wait", + $(LabelElement.class).first().getText()); + + waitUntil(new ExpectedCondition() { + @Override + public Object apply(WebDriver driver) { + return isElementPresent(ButtonElement.class); + } + }); + $(ButtonElement.class).id(HIDE).click(); + Assert.assertEquals("Please wait", + $(LabelElement.class).first().getText()); + } +} -- 2.39.5