From e16008b3719855d95cf0bb3402a0ac67eb319d47 Mon Sep 17 00:00:00 2001 From: Richard Braakman Date: Wed, 26 Sep 2012 03:01:53 +0300 Subject: [PATCH 1/3] Fix sessioninfos race that can cause crash during USER_CHANGES handling When stress testing etherpad-lite we occasionally got this error: TypeError: Cannot read property 'author' of undefined at /home/etherpad/etherpad-lite/src/node/handler/PadMessageHandler.js:556:47 handleUserChanges was accessing sessioninfos[client.id].author in a callback, after spending some time in the loop that updates the changeset to the latest revision. It's possible for a disconnect request to be processed during that loop so the session might no longer be there. This patch fixes it by looking up the author at the start of the function. --- src/node/handler/PadMessageHandler.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 913433b0..16068680 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -465,6 +465,7 @@ function handleUserChanges(client, message) var baseRev = message.data.baseRev; var wireApool = (new AttributePool()).fromJsonable(message.data.apool); var changeset = message.data.changeset; + var thisAuthor = sessioninfos[client.id].author; var r, apool, pad; @@ -545,8 +546,6 @@ function handleUserChanges(client, message) return; } - var thisAuthor = sessioninfos[client.id].author; - pad.appendRevision(changeset, thisAuthor); var correctionChangeset = _correctMarkersInPad(pad.atext, pad.pool); From f1b4206cadb716dd3c4b2c924e0b988431034263 Mon Sep 17 00:00:00 2001 From: Richard Braakman Date: Wed, 26 Sep 2012 03:01:24 +0300 Subject: [PATCH 2/3] Fix crash when client submits changeset based on too-old revision We had a problem with the server running out of stack space if a client submitted a changeset based on a revision more than about 1000 revs old. (944 was our cutoff but yours may vary). This happened in the wild with about 30 people editing via flaky wifi. A disconnected client would try to submit a fairly old changeset when reconnecting, and a few minutes was enough for 30 people to generate that many revs. The stack kept growing because pad.getRevisionChangeset was being answered from the cache, so no I/O interrupted the callback chain. (This was seen with mysql, I don't know about other backends.) This patch forces a nextTick every 200 revisions to solve this problem. --- src/node/handler/PadMessageHandler.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 16068680..6a462b01 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -526,7 +526,11 @@ function handleUserChanges(client, message) if(ERR(err, callback)) return; changeset = Changeset.follow(c, changeset, false, apool); - callback(null); + if ((r - baseRev) % 200 == 0) { // don't let the stack get too deep + async.nextTick(callback); + } else { + callback(null); + } }); }, //use the callback of the series function From 7aaef01346ca61d7778c15f9b35f72713065f297 Mon Sep 17 00:00:00 2001 From: Richard Braakman Date: Thu, 27 Sep 2012 23:05:18 +0300 Subject: [PATCH 3/3] Prettify session handling in handleUserChanges Also add a comment to explain what's going on with thisSession. No changes in behavior. --- src/node/handler/PadMessageHandler.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 6a462b01..8a5a92bb 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -465,7 +465,9 @@ function handleUserChanges(client, message) var baseRev = message.data.baseRev; var wireApool = (new AttributePool()).fromJsonable(message.data.apool); var changeset = message.data.changeset; - var thisAuthor = sessioninfos[client.id].author; + // The client might disconnect between our callbacks. We should still + // finish processing the changeset, so keep a reference to the session. + var thisSession = sessioninfos[client.id]; var r, apool, pad; @@ -473,7 +475,7 @@ function handleUserChanges(client, message) //get the pad function(callback) { - padManager.getPad(sessioninfos[client.id].padId, function(err, value) + padManager.getPad(thisSession.padId, function(err, value) { if(ERR(err, callback)) return; pad = value; @@ -550,7 +552,7 @@ function handleUserChanges(client, message) return; } - pad.appendRevision(changeset, thisAuthor); + pad.appendRevision(changeset, thisSession.author); var correctionChangeset = _correctMarkersInPad(pad.atext, pad.pool); if (correctionChangeset) {