From 0a2147aba4b9913776480110a5934ddfc5337a6c Mon Sep 17 00:00:00 2001 From: Gilberto Torrezan Date: Mon, 16 Jul 2018 14:01:47 +0300 Subject: [PATCH] Make inactive nodes mark their children as dirty on setInactive (#4397) * Make inactive nodes mark their children as dirty on setInactive This moves the setting of dirty nodes out of the collectChanges method. When using long-polling push, the client keeps asking for changes from the server. The server then collects all the changes and sends to the client. The problem is: a child node is always considered `dirty`, if its parent is invisible, and the node is always included in the changes sent to the client. Since there's not a single moment where there's nothing to send, the server keeps sending the changes, and the client keeps asking for the changes. This creates a busy loop between client and server, which only ends when the parent node is made visible again. To prevent this, this patch changes this logic to make the children nodes only dirty when the parent visibility changes. Fix #4353 --- .../com/vaadin/flow/internal/StateNode.java | 47 +++++++++++-------- .../vaadin/flow/internal/StateNodeTest.java | 39 ++++++++------- .../flow/uitest/ui/LongPollingPushView.java | 45 ++++++++++++++++++ .../flow/uitest/ui/LongPollingPushIT.java | 44 +++++++++++++++++ 4 files changed, 137 insertions(+), 38 deletions(-) create mode 100644 flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java create mode 100644 flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java diff --git a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java index d94f23ed6d7..9ab9ac6f3d5 100644 --- a/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java +++ b/flow-server/src/main/java/com/vaadin/flow/internal/StateNode.java @@ -378,25 +378,6 @@ public void collectChanges(Consumer collector) { return; } if (isInactive()) { - if (!isInactiveSelf) { - /* - * We are here if: the node itself is not inactive but it has - * some ascendant which is inactive. - * - * In this case we send only some subset of changes (not from - * all the features). But we should send changes for all - * remaining features. Normally it automatically happens if the - * node becomes "visible". But if it was visible with some - * invisible parent then only the parent becomes dirty (when - * it's set visible) and this child will never participate in - * collection of changes since it's not marked as dirty. - * - * So here such node (which is active itself but its ascendant - * is inactive) we mark as dirty again to be able to collect its - * changes later on when its ascendant becomes active. - */ - getOwner().markAsDirty(this); - } if (isInitialChanges) { // send only required (reported) features updates Stream initialFeatures = Stream @@ -712,7 +693,33 @@ private Stream getDisalowFeatures() { } private void setInactive(boolean inactive) { - isInactiveSelf = inactive; + if (isInactiveSelf != inactive) { + isInactiveSelf = inactive; + + visitNodeTree(child -> { + if (!this.equals(child) && !child.isInactiveSelf) { + /* + * We are here if: the child node itself is not inactive but + * it has some ascendant which is inactive. + * + * In this case we send only some subset of changes (not + * from all the features). But we should send changes for + * all remaining features. Normally it automatically happens + * if the node becomes "visible". But if it was visible with + * some invisible parent then only the parent becomes dirty + * (when it's set visible) and this child will never + * participate in collection of changes since it's not + * marked as dirty. + * + * So here such node (which is active itself but its + * ascendant is inactive) we mark as dirty again to be able + * to collect its changes later on when its ascendant + * becomes active. + */ + child.markAsDirty(); + } + }); + } } /** diff --git a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java index 8700e18d661..c2137e866a6 100644 --- a/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/internal/StateNodeTest.java @@ -659,8 +659,13 @@ public void collectChanges_initiallyInactiveViaParentElement_sendOnlyDisalowAndR private void assertCollectChanges_initiallyInactive(StateNode stateNode, ElementPropertyMap properties, Consumer activityUpdater) { - ElementData visibility = stateNode.getFeature(ElementData.class); + properties.setProperty("foo", "bar"); + + TestStateTree tree = (TestStateTree) stateNode.getOwner(); + tree.dirtyNodes.clear(); + + ElementData visibility = stateNode.getFeature(ElementData.class); activityUpdater.accept(false); // activity updater may modify visibility of the node itself or its @@ -668,22 +673,18 @@ private void assertCollectChanges_initiallyInactive(StateNode stateNode, // node is visible or not boolean visibilityChanged = !visibility.isVisible(); - properties.setProperty("foo", "bar"); - - TestStateTree tree = (TestStateTree) stateNode.getOwner(); - - tree.dirtyNodes.clear(); - List changes = new ArrayList<>(); stateNode.collectChanges(changes::add); if (visibilityChanged) { - Assert.assertEquals(0, tree.dirtyNodes.size()); + Assert.assertEquals(1, tree.dirtyNodes.size()); + Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode)); } else { // the target node should be marked as dirty because it's visible // but its parent is inactive - Assert.assertEquals(1, tree.dirtyNodes.size()); - tree.dirtyNodes.contains(stateNode); + Assert.assertEquals(2, tree.dirtyNodes.size()); + stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes, + CoreMatchers.hasItem(node))); } Assert.assertEquals(visibilityChanged ? 3 : 2, changes.size()); @@ -699,7 +700,6 @@ private void assertCollectChanges_initiallyInactive(StateNode stateNode, Assert.assertTrue("No tag change found", tagFound.isPresent()); MapPutChange tagChange = tagFound.get(); - MapPutChange change = (MapPutChange) changes.get(1); if (visibilityChanged) { Assert.assertThat(changes.get(2), @@ -784,6 +784,9 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode, changes.clear(); + TestStateTree tree = (TestStateTree) stateNode.getOwner(); + tree.dirtyNodes.clear(); + // now make the node inactive via the VisibiltyData activityUpdater.accept(false); @@ -793,9 +796,6 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode, properties.setProperty("foo", "baz"); - TestStateTree tree = (TestStateTree) stateNode.getOwner(); - tree.dirtyNodes.clear(); - stateNode.collectChanges(changes::add); // activity updater may modify visibility of the node itself or its @@ -808,7 +808,8 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode, MapPutChange change; if (visibilityChanged) { - Assert.assertEquals(0, tree.dirtyNodes.size()); + Assert.assertEquals(1, tree.dirtyNodes.size()); + Assert.assertThat(tree.dirtyNodes, CoreMatchers.hasItem(stateNode)); Assert.assertThat(changes.get(0), CoreMatchers.instanceOf(MapPutChange.class)); change = (MapPutChange) changes.get(0); @@ -816,8 +817,9 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode, } else { // the target node should be marked as dirty because it's visible // but its parent is inactive - Assert.assertEquals(1, tree.dirtyNodes.size()); - tree.dirtyNodes.contains(stateNode); + Assert.assertEquals(2, tree.dirtyNodes.size()); + stateNode.visitNodeTree(node -> Assert.assertThat(tree.dirtyNodes, + CoreMatchers.hasItem(node))); } changes.clear(); @@ -841,7 +843,8 @@ private void assertCollectChanges_initiallyVisible(StateNode stateNode, .equals(change.getFeature()) ? change : (MapPutChange) changes.get(1); propertyChange = change.equals(visibilityChange) - ? (MapPutChange) changes.get(1) : change; + ? (MapPutChange) changes.get(1) + : change; } else { propertyChange = change; } diff --git a/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java new file mode 100644 index 00000000000..3411d9c9494 --- /dev/null +++ b/flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/LongPollingPushView.java @@ -0,0 +1,45 @@ +/* + * Copyright 2000-2018 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.flow.uitest.ui; + +import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.html.NativeButton; +import com.vaadin.flow.component.html.Span; +import com.vaadin.flow.component.page.Push; +import com.vaadin.flow.router.Route; +import com.vaadin.flow.shared.ui.Transport; + +/** + * Class for reproducing the bug https://github.com/vaadin/flow/issues/4353 + */ +@Route("com.vaadin.flow.uitest.ui.LongPollingPushView") +@Push(transport = Transport.LONG_POLLING) +public class LongPollingPushView extends AbstractDivView { + + public LongPollingPushView() { + Div parent = new Div(); + Span child = new Span("Some text"); + child.setId("child"); + parent.add(child); + add(parent); + parent.setVisible(false); + + NativeButton visibility = new NativeButton("Toggle visibility", + e -> parent.setVisible(!parent.isVisible())); + visibility.setId("visibility"); + add(visibility); + } +} diff --git a/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java new file mode 100644 index 00000000000..9f5d9693c61 --- /dev/null +++ b/flow-tests/test-root-context/src/test/java/com/vaadin/flow/uitest/ui/LongPollingPushIT.java @@ -0,0 +1,44 @@ +/* + * Copyright 2000-2018 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.flow.uitest.ui; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; + +import com.vaadin.flow.testutil.ChromeBrowserTest; + +public class LongPollingPushIT extends ChromeBrowserTest { + + @Test + public void openPage_thereAreNoErrorsInTheConsole() { + open(); + checkLogsForErrors(); + + waitForElementNotPresent(By.id("child")); + + WebElement button = findElement(By.id("visibility")); + button.click(); + + waitForElementPresent(By.id("child")); + + WebElement span = findElement(By.id("child")); + Assert.assertEquals("Some text", span.getText()); + checkLogsForErrors(); + } + +}