Merge pull request #1028 from amtep/develop

Fix server crash if client disconnects too soon after connecting

src/node/handler/PadMessageHandler.js

@@ -436,15 +436,22 @@ function handleUserInfoUpdate(client, message)
 }
 
 /**
- * Handles a USERINFO_UPDATE, that means that a user have changed his color or name. Anyway, we get both informations
+ * Handles a USER_CHANGES message, where the client submits its local
- * This Method is nearly 90% copied out of the Etherpad Source Code. So I can't tell you what happens here exactly
+ * edits as a changeset.
- * Look at https://github.com/ether/pad/blob/master/etherpad/src/etherpad/collab/collab_server.js in the function applyUserChanges()
+ *
+ * This handler's job is to update the incoming changeset so that it applies
+ * to the latest revision, then add it to the pad, broadcast the changes
+ * to all other clients, and send a confirmation to the submitting client.
+ *
+ * This function is based on a similar one in the original Etherpad.
+ *   See https://github.com/ether/pad/blob/master/etherpad/src/etherpad/collab/collab_server.js in the function applyUserChanges()
+ *
  * @param client the client that send this message
  * @param message the message from the client
  */
 function handleUserChanges(client, message)
 {
-  //check if all ok
+  // Make sure all required fields are present
   if(message.data.baseRev == null)
   {
     messageLogger.warn("Dropped message, USER_CHANGES Message has no baseRev!");
@@ -487,22 +494,23 @@ function handleUserChanges(client, message)
     {
       //ex. _checkChangesetAndPool
   
-      //Copied from Etherpad, don't know what it does exactly
       try
       {
-        //this looks like a changeset check, it throws errors sometimes
+        // Verify that the changeset has valid syntax and is in canonical form
         Changeset.checkRep(changeset);
-      
+
+        // Verify that the attribute indexes used in the changeset are all
+        // defined in the accompanying attribute pool.
         Changeset.eachAttribNumber(changeset, function(n) {
           if (! wireApool.getAttrib(n)) {
             throw "Attribute pool is missing attribute "+n+" for changeset "+changeset;
           }
         });
       }
-      //there is an error in this changeset, so just refuse it
       catch(e)
       {
-        console.warn("Can't apply USER_CHANGES "+changeset+", cause it faild checkRep");
+        // There is an error in this changeset, so just refuse it
+        console.warn("Can't apply USER_CHANGES "+changeset+", because it failed checkRep");
         client.json.send({disconnect:"badChangeset"});
         return;
       }
@@ -515,7 +523,10 @@ function handleUserChanges(client, message)
       //ex. applyUserChanges
       apool = pad.pool;
       r = baseRev;
-        
+
+      // The client's changeset might not be based on the latest revision,
+      // since other clients are sending changes at the same time.
+      // Update the changeset so that it can be applied to the latest revision.
       //https://github.com/caolan/async#whilst
       async.whilst(
         function() { return r < pad.getHeadRevisionNumber(); },
@@ -526,8 +537,13 @@ function handleUserChanges(client, message)
           pad.getRevisionChangeset(r, function(err, c)
           {
             if(ERR(err, callback)) return;
-            
+
+            // At this point, both "c" (from the pad) and "changeset" (from the
+            // client) are relative to revision r - 1. The follow function
+            // rebases "changeset" so that it is relative to revision r
+            // and can be applied after "c".
             changeset = Changeset.follow(c, changeset, false, apool);
+
             if ((r - baseRev) % 200 == 0) { // don't let the stack get too deep
               async.nextTick(callback);
             } else {
@@ -558,7 +574,8 @@ function handleUserChanges(client, message)
       if (correctionChangeset) {
         pad.appendRevision(correctionChangeset);
       }
-        
+
+      // Make sure the pad always ends with an empty line.
       if (pad.text().lastIndexOf("\n\n") != pad.text().length-2) {
         var nlChangeset = Changeset.makeSplice(pad.text(), pad.text().length-1, 0, "\n");
         pad.appendRevision(nlChangeset);
@@ -865,6 +882,13 @@ function handleClientReady(client, message)
     },
     function(callback)
     {
+      //Check that the client is still here. It might have disconnected between callbacks.
+      if(sessioninfos[client.id] === undefined)
+      {
+        callback();
+        return;
+      }
+
       //Check if this author is already on the pad, if yes, kick the other sessions!
       if(pad2sessions[padIds.padId])
       {
@@ -879,10 +903,9 @@ function handleClientReady(client, message)
       }
       
       //Save in sessioninfos that this session belonges to this pad
-      var sessionId=String(client.id);
+      sessioninfos[client.id].padId = padIds.padId;
-      sessioninfos[sessionId].padId = padIds.padId;
+      sessioninfos[client.id].readOnlyPadId = padIds.readOnlyPadId;
-      sessioninfos[sessionId].readOnlyPadId = padIds.readOnlyPadId;
+      sessioninfos[client.id].readonly = padIds.readonly;
-      sessioninfos[sessionId].readonly = padIds.readonly;
       
       //check if there is already a pad2sessions entry, if not, create one
       if(!pad2sessions[padIds.padId])
@@ -891,7 +914,7 @@ function handleClientReady(client, message)
       }
       
       //Saves in pad2sessions that this session belongs to this pad
-      pad2sessions[padIds.padId].push(sessionId);
+      pad2sessions[padIds.padId].push(client.id);
       
       //prepare all values for the wire
       var atext = Changeset.cloneAText(pad.atext);
@@ -956,26 +979,22 @@ function handleClientReady(client, message)
         clientVars.userName = authorName;
       }
       
-      if(sessioninfos[client.id] !== undefined)
+      //If this is a reconnect, we don't have to send the client the ClientVars again
+      if(message.reconnect == true)
       {
-        //This is a reconnect, so we don't have to send the client the ClientVars again
+        //Save the revision in sessioninfos, we take the revision from the info the client send to us
-        if(message.reconnect == true)
+        sessioninfos[client.id].rev = message.client_rev;
-        {
-          //Save the revision in sessioninfos, we take the revision from the info the client send to us
-          sessioninfos[client.id].rev = message.client_rev;
-        }
-        //This is a normal first connect
-        else
-        {
-          //Send the clientVars to the Client
-          client.json.send({type: "CLIENT_VARS", data: clientVars});
-          //Save the revision in sessioninfos
-          sessioninfos[client.id].rev = pad.getHeadRevisionNumber();
-        }
-        
-        //Save the revision and the author id in sessioninfos
-        sessioninfos[client.id].author = author;
       }
+      //This is a normal first connect
+      else
+      {
+        //Send the clientVars to the Client
+        client.json.send({type: "CLIENT_VARS", data: clientVars});
+        //Save the current revision in sessioninfos, should be the same as in clientVars
+        sessioninfos[client.id].rev = pad.getHeadRevisionNumber();
+      }
+        
+      sessioninfos[client.id].author = author;
       
       //prepare the notification for the other users on the pad, that this user joined
       var messageToTheOtherUsers = {