Ignore RPC calls from components that are concurrently removed (#12337)

Change-Id: I8b97444d33b9535b9073fd705fed15a6cc2992e7

6 files changed, 347 insertions, 24 deletions

diff --git a/client/src/com/vaadin/client/ApplicationConnection.java b/client/src/com/vaadin/client/ApplicationConnection.java
index 364ce4521d..cd1f8a206d 100644
--- a/client/src/com/vaadin/client/ApplicationConnection.java
+++ b/client/src/com/vaadin/client/ApplicationConnection.java
@@ -49,6 +49,7 @@ import com.google.gwt.http.client.RequestException;
 import com.google.gwt.http.client.Response;
 import com.google.gwt.http.client.URL;
 import com.google.gwt.json.client.JSONArray;
+import com.google.gwt.json.client.JSONNumber;
 import com.google.gwt.json.client.JSONObject;
 import com.google.gwt.json.client.JSONString;
 import com.google.gwt.regexp.shared.MatchResult;
@@ -267,8 +268,6 @@ public class ApplicationConnection {
     /** Event bus for communication events */
     private EventBus eventBus = GWT.create(SimpleEventBus.class);
 
-    private int lastResponseId = -1;
-
     /**
      * The communication handler methods are called at certain points during
      * communication with the server. This allows for making add-ons that keep
@@ -719,6 +718,8 @@ public class ApplicationConnection {
         payload.put(ApplicationConstants.CSRF_TOKEN, new JSONString(
                 getCsrfToken()));
         payload.put(ApplicationConstants.RPC_INVOCATIONS, reqInvocations);
+        payload.put(ApplicationConstants.SERVER_SYNC_ID, new JSONNumber(
+                lastSeenServerSyncId));
 
         VConsole.log("Making UIDL Request with params: " + payload);
         String uri = translateVaadinUri(ApplicationConstants.APP_PROTOCOL_PREFIX
@@ -978,6 +979,29 @@ public class ApplicationConnection {
      */
     private ValueMap serverTimingInfo;
 
+    /**
+     * Holds the last seen response id given by the server.
+     * <p>
+     * The server generates a strictly increasing id for each response to each
+     * request from the client. This ID is then replayed back to the server on
+     * each request. This helps the server in knowing in what state the client
+     * is, and compare it to its own state. In short, it helps with concurrent
+     * changes between the client and server.
+     * <p>
+     * Initial value, i.e. no responses received from the server, is
+     * {@link #UNDEFINED_SYNC_ID} ({@value #UNDEFINED_SYNC_ID}). This happens
+     * between the bootstrap HTML being loaded and the first UI being rendered;
+     */
+    private int lastSeenServerSyncId = UNDEFINED_SYNC_ID;
+
+    /**
+     * The value of an undefined sync id.
+     * <p>
+     * This must be <code>-1</code>, because of the contract in
+     * {@link #getLastResponseId()}
+     */
+    private static final int UNDEFINED_SYNC_ID = -1;
+
     static final int MAX_CSS_WAITS = 100;
 
     protected void handleWhenCSSLoaded(final String jsonText,
@@ -1258,7 +1282,13 @@ public class ApplicationConnection {
      * @return and id identifying the response
      */
     public int getLastResponseId() {
-        return lastResponseId;
+        /*
+         * The discrepancy between field name and getter name is simply historic
+         * - API can't be changed, but the field was repurposed in a more
+         * general, yet compatible, use. "Response id" was deemed unsuitable a
+         * name, so it was called "server sync id" instead.
+         */
+        return lastSeenServerSyncId;
     }
 
     protected void handleUIDLMessage(final Date start, final String jsonText,
@@ -1285,6 +1315,17 @@ public class ApplicationConnection {
         VConsole.log("Handling message from server");
         eventBus.fireEvent(new ResponseHandlingStartedEvent(this));
 
+        if (json.containsKey(ApplicationConstants.SERVER_SYNC_ID)) {
+            int syncId = json.getInt(ApplicationConstants.SERVER_SYNC_ID);
+
+            assert (lastSeenServerSyncId == UNDEFINED_SYNC_ID || syncId == lastSeenServerSyncId + 1) : "Newly retrieved server sync id was not exactly one larger than the previous one (new: "
+                    + syncId + ", last seen: " + lastSeenServerSyncId + ")";
+
+            lastSeenServerSyncId = syncId;
+        } else {
+            VConsole.error("Server response didn't contain an id.");
+        }
+
         // Handle redirect
         if (json.containsKey("redirect")) {
             String url = json.getValueMap("redirect").getString("url");
@@ -1293,8 +1334,6 @@ public class ApplicationConnection {
             return;
         }
 
-        lastResponseId++;
-
         final MultiStepDuration handleUIDLDuration = new MultiStepDuration();
 
         // Get security key
diff --git a/server/src/com/vaadin/server/communication/ServerRpcHandler.java b/server/src/com/vaadin/server/communication/ServerRpcHandler.java
index e52ef6d037..eff9ceebf4 100644
--- a/server/src/com/vaadin/server/communication/ServerRpcHandler.java
+++ b/server/src/com/vaadin/server/communication/ServerRpcHandler.java
@@ -72,11 +72,13 @@ public class ServerRpcHandler implements Serializable {
 
         private final String csrfToken;
         private final JSONArray invocations;
+        private final int syncId;
         private final JSONObject json;
 
         public RpcRequest(String jsonString) throws JSONException {
             json = new JSONObject(jsonString);
             csrfToken = json.getString(ApplicationConstants.CSRF_TOKEN);
+            syncId = json.getInt(ApplicationConstants.SERVER_SYNC_ID);
             invocations = new JSONArray(
                     json.getString(ApplicationConstants.RPC_INVOCATIONS));
         }
@@ -101,6 +103,16 @@ public class ServerRpcHandler implements Serializable {
         }
 
         /**
+         * Gets the sync id last seen by the client.
+         * 
+         * @return the last sync id given by the server, according to the
+         *         client's request
+         */
+        public int getSyncId() {
+            return syncId;
+        }
+
+        /**
          * Gets the entire request in JSON format, as it was received from the
          * client.
          * <p>
@@ -153,7 +165,11 @@ public class ServerRpcHandler implements Serializable {
                 rpcRequest.getCsrfToken())) {
             throw new InvalidUIDLSecurityKeyException("");
         }
-        handleInvocations(ui, rpcRequest.getRpcInvocationsData());
+        handleInvocations(ui, rpcRequest.getSyncId(),
+                rpcRequest.getRpcInvocationsData());
+
+        ui.getConnectorTracker().cleanConcurrentlyRemovedConnectorIds(
+                rpcRequest.getSyncId());
     }
 
     /**
@@ -169,11 +185,15 @@ public class ServerRpcHandler implements Serializable {
      * 
      * @param uI
      *            the UI receiving the invocations data
+     * @param lastSyncIdSeenByClient
+     *            the most recent sync id the client has seen at the time the
+     *            request was sent
      * @param invocationsData
      *            JSON containing all information needed to execute all
      *            requested RPC calls.
      */
-    private void handleInvocations(UI uI, JSONArray invocationsData) {
+    private void handleInvocations(UI uI, int lastSyncIdSeenByClient,
+            JSONArray invocationsData) {
         // TODO PUSH Refactor so that this is not needed
         LegacyCommunicationManager manager = uI.getSession()
                 .getCommunicationManager();
@@ -182,7 +202,8 @@ public class ServerRpcHandler implements Serializable {
             Set<Connector> enabledConnectors = new HashSet<Connector>();
 
             List<MethodInvocation> invocations = parseInvocations(
-                    uI.getConnectorTracker(), invocationsData);
+                    uI.getConnectorTracker(), invocationsData,
+                    lastSyncIdSeenByClient);
             for (MethodInvocation invocation : invocations) {
                 final ClientConnector connector = manager.getConnector(uI,
                         invocation.getConnectorId());
@@ -302,12 +323,15 @@ public class ServerRpcHandler implements Serializable {
      * @param invocationsJson
      *            JSON containing all information needed to execute all
      *            requested RPC calls.
+     * @param lastSyncIdSeenByClient
+     *            the most recent sync id the client has seen at the time the
+     *            request was sent
      * @return list of MethodInvocation to perform
      * @throws JSONException
      */
     private List<MethodInvocation> parseInvocations(
-            ConnectorTracker connectorTracker, JSONArray invocationsJson)
-            throws JSONException {
+            ConnectorTracker connectorTracker, JSONArray invocationsJson,
+            int lastSyncIdSeenByClient) throws JSONException {
         ArrayList<MethodInvocation> invocations = new ArrayList<MethodInvocation>();
 
         MethodInvocation previousInvocation = null;
@@ -317,7 +341,8 @@ public class ServerRpcHandler implements Serializable {
             JSONArray invocationJson = invocationsJson.getJSONArray(i);
 
             MethodInvocation invocation = parseInvocation(invocationJson,
-                    previousInvocation, connectorTracker);
+                    previousInvocation, connectorTracker,
+                    lastSyncIdSeenByClient);
             if (invocation != null) {
                 // Can be null if the invocation was a legacy invocation and it
                 // was merged with the previous one or if the invocation was
@@ -331,7 +356,8 @@ public class ServerRpcHandler implements Serializable {
 
     private MethodInvocation parseInvocation(JSONArray invocationJson,
             MethodInvocation previousInvocation,
-            ConnectorTracker connectorTracker) throws JSONException {
+            ConnectorTracker connectorTracker, long lastSyncIdSeenByClient)
+            throws JSONException {
         String connectorId = invocationJson.getString(0);
         String interfaceName = invocationJson.getString(1);
         String methodName = invocationJson.getString(2);
@@ -339,18 +365,22 @@ public class ServerRpcHandler implements Serializable {
         if (connectorTracker.getConnector(connectorId) == null
                 && !connectorId
                         .equals(ApplicationConstants.DRAG_AND_DROP_CONNECTOR_ID)) {
-            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();
+
+            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;
         }
 
diff --git a/server/src/com/vaadin/server/communication/UidlWriter.java b/server/src/com/vaadin/server/communication/UidlWriter.java
index 60933a75c2..b46fbbf58a 100644
--- a/server/src/com/vaadin/server/communication/UidlWriter.java
+++ b/server/src/com/vaadin/server/communication/UidlWriter.java
@@ -38,6 +38,7 @@ import com.vaadin.server.LegacyCommunicationManager;
 import com.vaadin.server.LegacyCommunicationManager.ClientCache;
 import com.vaadin.server.SystemMessages;
 import com.vaadin.server.VaadinSession;
+import com.vaadin.shared.ApplicationConstants;
 import com.vaadin.ui.ConnectorTracker;
 import com.vaadin.ui.UI;
 
@@ -98,6 +99,9 @@ public class UidlWriter implements Serializable {
 
         uiConnectorTracker.setWritingResponse(true);
         try {
+            writer.write("\"" + ApplicationConstants.SERVER_SYNC_ID
+                    + "\": " + uiConnectorTracker.getCurrentSyncId() + ", ");
+
             writer.write("\"changes\" : ");
 
             JsonPaintTarget paintTarget = new JsonPaintTarget(manager, writer,
diff --git a/server/src/com/vaadin/ui/ConnectorTracker.java b/server/src/com/vaadin/ui/ConnectorTracker.java
index 0f8ec60104..33d585adca 100644
--- a/server/src/com/vaadin/ui/ConnectorTracker.java
+++ b/server/src/com/vaadin/ui/ConnectorTracker.java
@@ -25,6 +25,7 @@ import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.Map;
 import java.util.Set;
+import java.util.TreeMap;
 import java.util.UUID;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -81,6 +82,16 @@ public class ConnectorTracker implements Serializable {
 
     private Map<StreamVariable, String> streamVariableToSeckey;
 
+    private int currentSyncId = 0;
+
+    /**
+     * Map to track on which syncId each connector was removed.
+     * 
+     * @see #getCurrentSyncId()
+     * @see #cleanConcurrentlyRemovedConnectorIds(long)
+     */
+    private TreeMap<Integer, Set<String>> syncIdToUnregisteredConnectorIds = new TreeMap<Integer, Set<String>>();
+
     /**
      * Gets a logger for this class
      * 
@@ -170,6 +181,15 @@ 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<String>();
+            syncIdToUnregisteredConnectorIds.put(currentSyncId,
+                    unregisteredConnectorIds);
+        }
+        unregisteredConnectorIds.add(connectorId);
+
         dirtyConnectors.remove(connector);
         if (unregisteredConnectors.add(connector)) {
             if (getLogger().isLoggable(Level.FINE)) {
@@ -570,12 +590,18 @@ public class ConnectorTracker implements Serializable {
     /**
      * Sets the current response write status. Connectors can not be marked as
      * dirty when the response is written.
+     * <p>
+     * This method has a side-effect of incrementing the sync id by one (see
+     * {@link #getCurrentSyncId()}), if {@link #isWritingResponse()} returns
+     * <code>false</code> and <code>writingResponse</code> is set to
+     * <code>true</code>.
      * 
      * @param writingResponse
      *            the new response status.
      * 
      * @see #markDirty(ClientConnector)
      * @see #isWritingResponse()
+     * @see #getCurrentSyncId()
      * 
      * @throws IllegalArgumentException
      *             if the new response status is the same as the previous value.
@@ -587,6 +613,14 @@ public class ConnectorTracker implements Serializable {
             throw new IllegalArgumentException(
                     "The old value is same as the new value");
         }
+
+        /*
+         * the right hand side of the && is unnecessary here because of the
+         * if-clause above, but rigorous coding is always rigorous coding.
+         */
+        if (writingResponse && !this.writingResponse) {
+            currentSyncId++;
+        }
         this.writingResponse = writingResponse;
     }
 
@@ -732,4 +766,105 @@ 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
+     * @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";
+
+        boolean clientRequestIsTooOld = lastSyncIdSeenByClient < currentSyncId;
+        if (clientRequestIsTooOld) {
+            /*
+             * The headMap call is present here because we're only interested in
+             * connectors removed "in the past" (i.e. the server has removed
+             * them before the client ever knew about that), since those are the
+             * ones that we choose to handle as a special case.
+             */
+            /*-
+             *   Server                          Client
+             * [#1 add table] ---------.
+             *                          \
+             * [push: #2 remove table]-. `--> [adding table, storing #1]
+             *                          \  .- [table from request #1 needs more data]
+             *                           \/
+             *                           /`-> [removing table, storing #2]
+             * [#1 < #2 - ignoring] <---´
+             */
+            for (Set<String> unregisteredConnectors : syncIdToUnregisteredConnectorIds
+                    .headMap(currentSyncId).values()) {
+                if (unregisteredConnectors.contains(connectorId)) {
+                    return true;
+                }
+            }
+        }
+
+        return false;
+    }
+
+    /**
+     * Gets the most recently generated server sync id.
+     * <p>
+     * The sync id is incremented by one whenever a new response is being
+     * written. This id is then sent over to the client. The client then adds
+     * the most recent sync id to each communication packet it sends back to the
+     * server. This way, the server knows at what state the client is when the
+     * packet is sent. If the state has changed on the server side since that,
+     * the server can try to adjust the way it handles the actions from the
+     * client side.
+     * 
+     * @see #setWritingResponse(boolean)
+     * @see #connectorWasPresentAsRequestWasSent(String, long)
+     * @since 7.2
+     * @return the current sync id
+     */
+    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.
+     * 
+     * @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) {
+        /*
+         * 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)
+                .clear();
+    }
 }
diff --git a/server/tests/src/com/vaadin/tests/server/component/table/TableRemovedQuicklySendsInvalidRpcCalls.java b/server/tests/src/com/vaadin/tests/server/component/table/TableRemovedQuicklySendsInvalidRpcCalls.java
new file mode 100644
index 0000000000..b539e42efe
--- /dev/null
+++ b/server/tests/src/com/vaadin/tests/server/component/table/TableRemovedQuicklySendsInvalidRpcCalls.java
@@ -0,0 +1,107 @@
+/*
+ * Copyright 2000-2013 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.server.component.table;
+
+import com.vaadin.annotations.Push;
+import com.vaadin.event.ItemClickEvent;
+import com.vaadin.event.ItemClickEvent.ItemClickListener;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Button.ClickEvent;
+import com.vaadin.ui.Table;
+
+@Push
+public class TableRemovedQuicklySendsInvalidRpcCalls extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        addComponent(new Button("Blink a table", new Button.ClickListener() {
+            @Override
+            public void buttonClick(ClickEvent event) {
+                blinkTable();
+            }
+        }));
+    }
+
+    private void blinkTable() {
+        final Table table = new Table();
+        table.setPageLength(5);
+        table.addContainerProperty(new Object(), String.class, null);
+
+        for (int i = 0; i < 50; i++) {
+            table.addItem(new Object[] { "Row" }, new Object());
+        }
+
+        table.addItemClickListener(new ItemClickListener() {
+            private int i;
+
+            @Override
+            public void itemClick(ItemClickEvent event) {
+                /*
+                 * Ignore implementation. This is only an easy way to make the
+                 * client-side update table's variables (by furiously clicking
+                 * on the table row.
+                 * 
+                 * This way, we get variable changes queued. The push call will
+                 * then remove the Table, while the variable changes being still
+                 * in the queue, leading to the issue as described in the
+                 * ticket.
+                 */
+                System.out.println("clicky " + (++i));
+            }
+        });
+
+        System.out.println("adding component");
+        addComponent(table);
+
+        new Thread() {
+            @Override
+            public void run() {
+                getSession().lock();
+                try {
+                    Thread.sleep(500);
+                    access(new Runnable() {
+                        @Override
+                        public void run() {
+                            System.out.println("removing component");
+                            removeComponent(table);
+                        }
+                    });
+                } catch (InterruptedException e) {
+                    e.printStackTrace();
+                } finally {
+                    getSession().unlock();
+                }
+            };
+        }.start();
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "Adding and subsequently quickly removing a table "
+                + "should not leave any pending RPC calls waiting "
+                + "in a Timer. Issue can be reproduced by "
+                + "1) pressing the button 2) clicking furiously "
+                + "on a row in the table.";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 12337;
+    }
+}
diff --git a/shared/src/com/vaadin/shared/ApplicationConstants.java b/shared/src/com/vaadin/shared/ApplicationConstants.java
index 1dff1e19cc..5ae52615bb 100644
--- a/shared/src/com/vaadin/shared/ApplicationConstants.java
+++ b/shared/src/com/vaadin/shared/ApplicationConstants.java
@@ -96,4 +96,12 @@ public class ApplicationConstants implements Serializable {
      * @since 7.2
      */
     public static final String CSRF_TOKEN = "csrfToken";
+
+    /**
+     * The name of the parameter used to transmit the sync id
+     * 
+     * @see com.vaadin.ui.ConnectorTracker#getCurrentSyncId()
+     * @since 7.2
+     */
+    public static final String SERVER_SYNC_ID = "syncId";
 }