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

Change-Id: I8b97444d33b9535b9073fd705fed15a6cc2992e7

3 files changed, 188 insertions, 19 deletions

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();
+    }
 }