From 413ddb393e98b966fa14c934753e63be18d81912 Mon Sep 17 00:00:00 2001 From: Richard Braakman Date: Fri, 28 Sep 2012 22:49:20 +0300 Subject: [PATCH 1/2] Add some explanatory comments to handleUserChanges() --- src/node/handler/PadMessageHandler.js | 41 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 8a5a92bb6..f60d91da6 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/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 - * This Method is nearly 90% copied out of the Etherpad Source Code. So I can't tell you what happens here exactly - * Look at https://github.com/ether/pad/blob/master/etherpad/src/etherpad/collab/collab_server.js in the function applyUserChanges() + * Handles a USER_CHANGES message, where the client submits its local + * edits as a changeset. + * + * 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); From 2e72a1e4895688bc4cc8340a9b49f306b7d41600 Mon Sep 17 00:00:00 2001 From: Richard Braakman Date: Fri, 28 Sep 2012 23:03:42 +0300 Subject: [PATCH 2/2] Prevent server crash in handleClientReady The client might have disconnected between callbacks so don't try to write to the session before checking this. The main callback of this function now has a single check at its top. Removed a redundant check halfway through the callback. Also normalized use of client.id for the session index instead of a mix of client.id and sessionId. Added some explanatory comments. --- src/node/handler/PadMessageHandler.js | 48 ++++++++++++++------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index f60d91da6..10b259ae2 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -882,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]) { @@ -896,10 +903,9 @@ function handleClientReady(client, message) } //Save in sessioninfos that this session belonges to this pad - var sessionId=String(client.id); - sessioninfos[sessionId].padId = padIds.padId; - sessioninfos[sessionId].readOnlyPadId = padIds.readOnlyPadId; - sessioninfos[sessionId].readonly = padIds.readonly; + sessioninfos[client.id].padId = padIds.padId; + sessioninfos[client.id].readOnlyPadId = padIds.readOnlyPadId; + sessioninfos[client.id].readonly = padIds.readonly; //check if there is already a pad2sessions entry, if not, create one if(!pad2sessions[padIds.padId]) @@ -908,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); @@ -973,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 - if(message.reconnect == true) - { - //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; + //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 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 = {