Remove tracking of unregistered connectors (#8153)

client/src/main/java/com/vaadin/client/communication/MessageHandler.java

@@ -486,13 +486,9 @@ public class MessageHandler {
                             .handleServerResponse(json.getValueMap("dd"));
                 }
 
-                int removed = unregisterRemovedConnectors(
+                unregisterRemovedConnectors(
                         connectorHierarchyUpdateResult.detachedConnectorIds);
-                if (removed > 0 && !isResponse(json)) {
-                    // Must acknowledge the removal using an XHR or server
-                    // memory usage will keep growing
-                    getUIConnector().sendAck();
-                }
+
                 getLogger().info("handleUIDLMessage: "
                         + (Duration.currentTimeMillis() - processUidlStart)
                         + " ms");
@@ -802,15 +798,14 @@ public class MessageHandler {
                         "verifyConnectorHierarchy - this is only performed in debug mode");
             }
 
-            private int unregisterRemovedConnectors(
+            private void unregisterRemovedConnectors(
                     FastStringSet detachedConnectors) {
                 Profiler.enter("unregisterRemovedConnectors");
 
                 JsArrayString detachedArray = detachedConnectors.dump();
-                int nrDetached = detachedArray.length();
-                for (int i = 0; i < nrDetached; i++) {
-                    ServerConnector connector = getConnectorMap()
-                            .getConnector(detachedArray.get(i));
+                for (int i = 0; i < detachedArray.length(); i++) {
+                    ServerConnector connector = getConnectorMap().getConnector(
+                            detachedArray.get(i));
 
                     Profiler.enter(
                             "unregisterRemovedConnectors unregisterConnector");
@@ -825,10 +820,9 @@ public class MessageHandler {
                     verifyConnectorHierarchy();
                 }
 
-                getLogger()
-                        .info("* Unregistered " + nrDetached + " connectors");
+                getLogger().info("* Unregistered " + detachedArray.length()
+                        + " connectors");
                 Profiler.leave("unregisterRemovedConnectors");
-                return nrDetached;
             }
 
             private JsArrayString createConnectorsIfNeeded(ValueMap json) {

client/src/main/java/com/vaadin/client/ui/ui/UIConnector.java

@@ -1164,16 +1164,6 @@ public class UIConnector extends AbstractSingleComponentContainerConnector
         return Logger.getLogger(UIConnector.class.getName());
     }
 
-    /**
-     * Send an acknowledgement RPC to the server. This allows the server to know
-     * which messages the client has received, even when the client is not
-     * sending any other traffic.
-     */
-    public void sendAck() {
-        getRpcProxy(UIServerRpc.class).acknowledge();
-
-    }
-
     private void setWindowOrderAndPosition() {
         if (windowOrderRegistration != null) {
             windowOrderRegistration.removeHandler();
@@ -1218,5 +1208,5 @@ public class UIConnector extends AbstractSingleComponentContainerConnector
             Collections.sort(result, this);
             return result;
         }
-    };
+    }
 }

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

@@ -109,6 +109,7 @@ public abstract class AbstractExtension extends AbstractClientConnector
      *            The parent to set
      */
     private void internalSetParent(ClientConnector parent) {
+        ClientConnector oldParent = getParent();
 
         // Send a detach event if the component is currently attached
         if (isAttached()) {
@@ -123,6 +124,9 @@ public abstract class AbstractExtension extends AbstractClientConnector
             attach();
         }
 
+        if (oldParent != null) {
+            oldParent.markAsDirty();
+        }
     }
 
     @Override

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

@@ -275,9 +275,6 @@ public class ServerRpcHandler implements Serializable {
                     rpcRequest.getRpcInvocationsData());
         }
 
-        ui.getConnectorTracker()
-                .cleanConcurrentlyRemovedConnectorIds(rpcRequest.getSyncId());
-
         if (rpcRequest.isResynchronize()) {
             ui.getSession().getCommunicationManager().repaintAll(ui);
         }
@@ -529,22 +526,6 @@ public class ServerRpcHandler implements Serializable {
         String interfaceName = invocationJson.getString(1);
         String methodName = invocationJson.getString(2);
 
-        if (connectorTracker.getConnector(connectorId) == null && !connectorId
-                .equals(ApplicationConstants.DRAG_AND_DROP_CONNECTOR_ID)) {
-
-            if (!connectorTracker.connectorWasPresentAsRequestWasSent(
-                    connectorId, lastSyncIdSeenByClient)) {
-                getLogger().log(Level.WARNING, "RPC call to " + interfaceName
-                        + "." + methodName + " received for connector "
-                        + connectorId
-                        + " but no such connector could be found. Resynchronizing client.");
-                // This is likely an out of sync issue (client tries to update a
-                // connector which is not present). Force resync.
-                connectorTracker.markAllConnectorsDirty();
-            }
-            return null;
-        }
-
         JsonArray parametersJson = invocationJson.getArray(3);
 
         if (LegacyChangeVariablesInvocation

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

@@ -43,6 +43,7 @@ import com.vaadin.event.ContextClickEvent.ContextClickListener;
 import com.vaadin.event.ContextClickEvent.ContextClickNotifier;
 import com.vaadin.event.ShortcutListener;
 import com.vaadin.server.AbstractClientConnector;
+import com.vaadin.server.ClientConnector;
 import com.vaadin.server.ComponentSizeValidator;
 import com.vaadin.server.ErrorMessage;
 import com.vaadin.server.ErrorMessage.ErrorLevel;
@@ -541,6 +542,8 @@ public abstract class AbstractComponent extends AbstractClientConnector
                     getClass().getName() + " already has a parent.");
         }
 
+        ClientConnector oldParent = getParent();
+
         // Send a detach event if the component is currently attached
         if (isAttached()) {
             detach();
@@ -553,6 +556,10 @@ public abstract class AbstractComponent extends AbstractClientConnector
         if (isAttached()) {
             attach();
         }
+
+        if (oldParent != null) {
+            oldParent.markAsDirty();
+        }
     }
 
     /**

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

@@ -24,9 +24,7 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.Map;
-import java.util.NavigableMap;
 import java.util.Set;
-import java.util.TreeMap;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -90,14 +88,6 @@ public class ConnectorTracker implements Serializable {
 
     private int currentSyncId = 0;
 
-    /**
-     * Map to track on which syncId each connector was removed.
-     *
-     * @see #getCurrentSyncId()
-     * @see #cleanConcurrentlyRemovedConnectorIds(int)
-     */
-    private final TreeMap<Integer, Set<String>> syncIdToUnregisteredConnectorIds = new TreeMap<>();
-
     /**
      * Gets a logger for this class
      *
@@ -182,15 +172,6 @@ public class ConnectorTracker implements Serializable {
                     + " is not the one that was registered for that id");
         }
 
-        Set<String> unregisteredConnectorIds = syncIdToUnregisteredConnectorIds
-                .get(currentSyncId);
-        if (unregisteredConnectorIds == null) {
-            unregisteredConnectorIds = new HashSet<>();
-            syncIdToUnregisteredConnectorIds.put(currentSyncId,
-                    unregisteredConnectorIds);
-        }
-        unregisteredConnectorIds.add(connectorId);
-
         dirtyConnectors.remove(connector);
         if (unregisteredConnectors.add(connector)) {
             if (getLogger().isLoggable(Level.FINE)) {
@@ -851,48 +832,6 @@ public class ConnectorTracker implements Serializable {
         return streamVariableToSeckey.get(variable);
     }
 
-    /**
-     * Check whether a connector was present on the client when the it was
-     * creating this request, but was removed server-side before the request
-     * arrived.
-     *
-     * @since 7.2
-     * @param connectorId
-     *            The connector id to check for whether it was removed
-     *            concurrently or not.
-     * @param lastSyncIdSeenByClient
-     *            the most recent sync id the client has seen at the time the
-     *            request was sent, or -1 to ignore potential problems
-     * @return <code>true</code> if the connector was removed before the client
-     *         had a chance to react to it.
-     */
-    public boolean connectorWasPresentAsRequestWasSent(String connectorId,
-            long lastSyncIdSeenByClient) {
-        assert getConnector(connectorId) == null : "Connector " + connectorId
-                + " is still attached";
-
-        if (lastSyncIdSeenByClient == -1) {
-            // Ignore potential problems
-            return true;
-        }
-
-        /*
-         * Use non-inclusive tail map to find all connectors that were removed
-         * after the reported sync id was sent to the client.
-         */
-        NavigableMap<Integer, Set<String>> unregisteredAfter = syncIdToUnregisteredConnectorIds
-                .tailMap(Integer.valueOf((int) lastSyncIdSeenByClient), false);
-        for (Set<String> unregisteredIds : unregisteredAfter.values()) {
-            if (unregisteredIds.contains(connectorId)) {
-                // Removed with a higher sync id, so it was most likely present
-                // when this sync id was sent.
-                return true;
-            }
-        }
-
-        return false;
-    }
-
     /**
      * Gets the most recently generated server sync id.
      * <p>
@@ -915,45 +854,4 @@ public class ConnectorTracker implements Serializable {
     public int getCurrentSyncId() {
         return currentSyncId;
     }
-
-    /**
-     * Maintains the bookkeeping connector removal and concurrency by removing
-     * entries that have become too old.
-     * <p>
-     * <em>It is important to run this call for each transmission from the
-     * client</em> , otherwise the bookkeeping gets out of date and the results
-     * form {@link #connectorWasPresentAsRequestWasSent(String, long)} will
-     * become invalid (that is, even though the client knew the component was
-     * removed, the aforementioned method would start claiming otherwise).
-     * <p>
-     * Entries that both client and server agree upon are removed. Since
-     * argument is the last sync id that the client has seen from the server, we
-     * know that entries earlier than that cannot cause any problems anymore.
-     * <p>
-     * The sync id value <code>-1</code> is ignored to facilitate testing with
-     * pre-recorded requests.
-     *
-     * @see #connectorWasPresentAsRequestWasSent(String, long)
-     * @since 7.2
-     * @param lastSyncIdSeenByClient
-     *            the sync id the client has most recently received from the
-     *            server.
-     */
-    public void cleanConcurrentlyRemovedConnectorIds(
-            int lastSyncIdSeenByClient) {
-        if (lastSyncIdSeenByClient == -1) {
-            // Sync id checking is not in use, so we should just clear the
-            // entire map to avoid leaking memory
-            syncIdToUnregisteredConnectorIds.clear();
-            return;
-        }
-        /*
-         * We remove all entries _older_ than the one reported right now,
-         * because the remaining still contain components that might cause
-         * conflicts. In any case, it's better to clean up too little than too
-         * much, especially as the data will hardly grow into the kilobytes.
-         */
-        syncIdToUnregisteredConnectorIds.headMap(lastSyncIdSeenByClient, true)
-                .clear();
-    }
 }

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

@@ -189,11 +189,6 @@ public abstract class UI extends AbstractSingleComponentContainer
             fireEvent(new PollEvent(UI.this));
         }
 
-        @Override
-        public void acknowledge() {
-            // Nothing to do, just need the message to be sent and processed
-        }
-
         @Override
         public void popstate(String uri) {
             getPage().updateLocation(uri, true, true);

server/src/test/java/com/vaadin/tests/server/abstractextension/AbstractExtensionSetParentTest.java

@@ -0,0 +1,23 @@
+package com.vaadin.tests.server.abstractextension;
+
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import com.vaadin.server.AbstractExtension;
+import com.vaadin.server.ClientConnector;
+
+public class AbstractExtensionSetParentTest {
+
+    private static class TestExtension extends AbstractExtension {
+
+    }
+
+    @Test
+    public void setParent_marks_old_parent_as_dirty() {
+        ClientConnector connector = Mockito.mock(ClientConnector.class);
+        TestExtension extension = new TestExtension();
+        extension.setParent(connector);
+        extension.setParent(null);
+        Mockito.verify(connector, Mockito.times(1)).markAsDirty();
+    }
+}

server/src/test/java/com/vaadin/tests/server/component/abstractcomponent/AbstractComponentSetParentTest.java

@@ -0,0 +1,22 @@
+package com.vaadin.tests.server.component.abstractcomponent;
+
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import com.vaadin.ui.AbstractComponent;
+import com.vaadin.ui.HasComponents;
+
+public class AbstractComponentSetParentTest {
+
+    private static class TestComponent extends AbstractComponent {
+    }
+
+    @Test
+    public void setParent_marks_old_parent_as_dirty() {
+        HasComponents hasComponents = Mockito.mock(HasComponents.class);
+        TestComponent testComponent = new TestComponent();
+        testComponent.setParent(hasComponents);
+        testComponent.setParent(null);
+        Mockito.verify(hasComponents, Mockito.times(1)).markAsDirty();
+    }
+}

shared/src/main/java/com/vaadin/shared/ui/ui/UIServerRpc.java

@@ -38,6 +38,4 @@ public interface UIServerRpc extends ClickRpc, ServerRpc {
 
     public void popstate(String uri);
 
-    @NoLoadingIndicator
-    public void acknowledge();
 }

uitest/src/main/java/com/vaadin/tests/components/table/TableRemovedQuicklySendsInvalidRpcCalls.java

@@ -27,7 +27,11 @@ import com.vaadin.tests.components.AbstractReindeerTestUI;
 import com.vaadin.ui.Button;
 import com.vaadin.ui.Button.ClickEvent;
 import com.vaadin.ui.ConnectorTracker;
+<<<<<<< HEAD
 import com.vaadin.v7.ui.Table;
+=======
+import com.vaadin.ui.Table;
+>>>>>>> 62c0d73... Remove tracking of unregistered connectors (#8153)
 
 import elemental.json.JsonObject;
 
@@ -174,25 +178,11 @@ public class TableRemovedQuicklySendsInvalidRpcCalls
             return tracker.getSeckey(variable);
         }
 
-        @Override
-        public boolean connectorWasPresentAsRequestWasSent(String connectorId,
-                long lastSyncIdSeenByClient) {
-            return tracker.connectorWasPresentAsRequestWasSent(connectorId,
-                    lastSyncIdSeenByClient);
-        }
-
         @Override
         public int getCurrentSyncId() {
             return tracker.getCurrentSyncId();
         }
 
-        @Override
-        public void cleanConcurrentlyRemovedConnectorIds(
-                int lastSyncIdSeenByClient) {
-            tracker.cleanConcurrentlyRemovedConnectorIds(
-                    lastSyncIdSeenByClient);
-        }
-
         @Override
         public boolean equals(Object obj) {
             return tracker.equals(obj);

uitest/src/main/java/com/vaadin/tests/push/PushRemoveConnectors.java

@@ -1,87 +0,0 @@
-package com.vaadin.tests.push;
-
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.TimeUnit;
-
-import org.apache.commons.lang.SerializationUtils;
-
-import com.vaadin.annotations.Push;
-import com.vaadin.server.VaadinRequest;
-import com.vaadin.tests.components.AbstractTestUIWithLog;
-import com.vaadin.ui.AbstractOrderedLayout;
-import com.vaadin.ui.Button;
-import com.vaadin.ui.Button.ClickEvent;
-import com.vaadin.ui.Button.ClickListener;
-import com.vaadin.ui.CheckBox;
-import com.vaadin.ui.HorizontalLayout;
-import com.vaadin.ui.Label;
-
-@Push
-public class PushRemoveConnectors extends AbstractTestUIWithLog {
-
-    private transient final ScheduledExecutorService threadPool = Executors
-            .newScheduledThreadPool(5);
-    static final String START = "start";
-    static final String STOP = "stop";
-    private AbstractOrderedLayout verticalLayout;
-    private transient ScheduledFuture<?> task = null;
-
-    @Override
-    protected void setup(VaadinRequest request) {
-        final CheckBox pollingEnabled = new CheckBox("Polling enabled");
-        pollingEnabled.addValueChangeListener(event -> setPollInterval(
-                pollingEnabled.getValue() ? 1000 : -1));
-
-        Button start = new Button("start");
-        start.setId(START);
-        start.addClickListener(new ClickListener() {
-
-            @Override
-            public void buttonClick(ClickEvent event) {
-                task = threadPool.scheduleAtFixedRate(new Runnable() {
-                    @Override
-                    public void run() {
-                        access(new Runnable() {
-                            @Override
-                            public void run() {
-                                populate();
-                                log("Serialized session size: "
-                                        + getSessionSize());
-                            }
-                        });
-                    }
-                }, 1, 1, TimeUnit.SECONDS);
-            }
-        });
-        Button stop = new Button("stop");
-        stop.setId(STOP);
-        stop.addClickListener(new ClickListener() {
-            @Override
-            public void buttonClick(ClickEvent event) {
-                if (task != null) {
-                    task.cancel(true);
-                    task = null;
-                }
-
-            }
-        });
-        verticalLayout = new HorizontalLayout();
-        populate();
-        addComponents(pollingEnabled, start, stop, verticalLayout);
-    }
-
-    private void populate() {
-        verticalLayout.removeAllComponents();
-        for (int i = 0; i < 500; i++) {
-            Label l = new Label(".");
-            l.setSizeUndefined();
-            verticalLayout.addComponent(l);
-        }
-    }
-
-    private int getSessionSize() {
-        return SerializationUtils.serialize(getSession()).length;
-    }
-}

uitest/src/test/java/com/vaadin/tests/push/PushRemoveConnectorsTest.java

@@ -1,40 +0,0 @@
-package com.vaadin.tests.push;
-
-import org.junit.Assert;
-import org.junit.Ignore;
-import org.junit.Test;
-
-import com.vaadin.testbench.elements.ButtonElement;
-import com.vaadin.tests.tb3.SingleBrowserTest;
-
-// ignored as not really working and takes a very long time
-@Ignore
-public class PushRemoveConnectorsTest extends SingleBrowserTest {
-
-    @Test
-    public void testNoMemoryLeak() throws InterruptedException {
-        openTestURL();
-        $(ButtonElement.class).id(PushRemoveConnectors.START).click();
-        Thread.sleep(5000);
-        int last = getMemoryUsage();
-        int i = 0;
-        while (i++ < 10) {
-            Thread.sleep(5000);
-            int now = getMemoryUsage();
-            System.out.println("Memory usage: " + now);
-            if (last == now) {
-                break;
-            }
-
-            last = now;
-        }
-        $(ButtonElement.class).id(PushRemoveConnectors.STOP).click();
-
-        Assert.assertNotEquals(10, i);
-    }
-
-    private int getMemoryUsage() {
-        return Integer.parseInt(
-                getLogRow(0).replaceFirst(".*Serialized session size: ", ""));
-    }
-}