🐛 Allow getting Presence while it's being destroyed

There's currently an edge case involving trying to get presence in the
middle of being destroyed. For example, consider these synchronous
calls:

```js
var presence = connection.getPresence('channel')
presence.destroy()
var newPresence = connection.getPresence('channel')
```

Since `Presence` objects are per-channel singletons, the above code is
effectively the same as:

```js
var presence = connection.getPresence('channel')
presence.destroy()
var newPresence = presence
```

In other words, our "new" presence is actually already being destroyed.
What's worse, is that if we start trying to create local presence before
the `destroy()` callback is triggered, that gets "cleaned up" by the
initial call to `destroy()`:

```js
var presence = connection.getPresence('channel')
presence.destroy(() => {
  // by the time we reach here, my newly-created localPresence
  // will have been destroyed
})
var newPresence = connection.getPresence('channel')
var localPresence = newPresence.create('me')
```

This change attempts to address this edge case by adding a private
`_wantsDestroy` to our `Presence` instances, which is:

  - `false` on construction
  - set to `true` when calling `destroy()`
  - reset to `false` if calling `connection.getPresence()`

We then check this flag when performing the `destroy()` tidy-up, and
avoid tidying up if we have since reset the flag.

Note that there's a corner case within this edge case, surrounding
`LocalPresence` instances created while a `destroy()` is in progress.
Since we might create `LocalPresence` instances at any point during the
asynchronous `destroy()`, we define the following behaviour:

  - any `LocalPresence` instances that exist at the time of invoking
    `destroy()` will be destroyed, **even if** we subsequently cancel
    the destroy (by invoking `connection.getPresence()`)
  - any `LocalPresence` instances that are created *after* invoking
    `destroy()` will be kept if the destroy is cancelled
  - creating a new `LocalPresence` while `_wantDestroy === true` will
    throw, because clients shouldn't put themselves in this situation
    and the behaviour when tidying this up is undefined

Author: alecgibson

lib/client/connection.js

@@ -776,17 +776,21 @@ Connection.prototype._initialize = function(message) {
 
 Connection.prototype.getPresence = function(channel) {
   var connection = this;
-  return util.digOrCreate(this._presences, channel, function() {
+  var presence = util.digOrCreate(this._presences, channel, function() {
     return new Presence(connection, channel);
   });
+  presence._wantsDestroy = false;
+  return presence;
 };
 
 Connection.prototype.getDocPresence = function(collection, id) {
   var channel = DocPresence.channel(collection, id);
   var connection = this;
-  return util.digOrCreate(this._presences, channel, function() {
+  var presence = util.digOrCreate(this._presences, channel, function() {
     return new DocPresence(connection, collection, id);
   });
+  presence._wantsDestroy = false;
+  return presence;
 };
 
 Connection.prototype._sendPresenceAction = function(action, seq, presence) {

lib/client/presence/presence.js

@@ -24,6 +24,7 @@ function Presence(connection, channel) {
 
   this._remotePresenceInstances = {};
   this._subscriptionCallbacksBySeq = {};
+  this._wantsDestroy = false;
 }
 emitter.mixin(Presence);
 
@@ -36,17 +37,25 @@ Presence.prototype.unsubscribe = function(callback) {
 };
 
 Presence.prototype.create = function(id) {
+  if (this._wantsDestroy) {
+    throw new Error('Presence is being destroyed');
+  }
   id = id || hat();
   var localPresence = this._createLocalPresence(id);
   this.localPresences[id] = localPresence;
   return localPresence;
 };
 
 Presence.prototype.destroy = function(callback) {
+  this._wantsDestroy = true;
   var presence = this;
+  // Store these at the time of destruction: any LocalPresence on this
+  // instance at this time will be destroyed, but if the destroy is
+  // cancelled, any future LocalPresence objects will be kept.
+  // See: https://github.com/share/sharedb/pull/579
+  var localIds = Object.keys(presence.localPresences);
   this.unsubscribe(function(error) {
     if (error) return presence._callbackOrEmit(error, callback);
-    var localIds = Object.keys(presence.localPresences);
     var remoteIds = Object.keys(presence._remotePresenceInstances);
     async.parallel(
       [
@@ -56,13 +65,18 @@ Presence.prototype.destroy = function(callback) {
           }, next);
         },
         function(next) {
+          // We don't bother stashing the RemotePresence instances because
+          // they're not really bound to our local state: if we want to
+          // destroy, we destroy them all, but if we cancel the destroy,
+          // we'll want to keep them all
+          if (!presence._wantsDestroy) return next();
           async.each(remoteIds, function(presenceId, next) {
             presence._remotePresenceInstances[presenceId].destroy(next);
           }, next);
         }
       ],
       function(error) {
-        delete presence.connection._presences[presence.channel];
+        if (presence._wantsDestroy) delete presence.connection._presences[presence.channel];
         presence._callbackOrEmit(error, callback);
       }
     );

test/client/presence/presence.js

@@ -98,6 +98,118 @@ describe('Presence', function() {
     ], done);
   });
 
+  it('gets presence during a destroy', function(done) {
+    var localPresence1 = presence1.create('presence-1');
+    var presence2a;
+
+    async.series([
+      presence2.subscribe.bind(presence2),
+      function(next) {
+        presence2.destroy(errorHandler(done));
+        next();
+      },
+      function(next) {
+        presence2a = connection2.getPresence('test-channel');
+        presence2a.subscribe(function(error) {
+          next(error);
+        });
+      },
+      function(next) {
+        localPresence1.submit({index: 5}, errorHandler(done));
+        presence2a.once('receive', function() {
+          next();
+        });
+      }
+    ], done);
+  });
+
+  it('destroys old local presence but keeps new local presence when getting during destroy', function(done) {
+    presence2.create('presence-2');
+    var presence2a;
+
+    async.series([
+      presence2.subscribe.bind(presence2),
+      function(next) {
+        presence2.destroy(function() {
+          expect(presence2).to.equal(presence2a);
+          expect(Object.keys(presence2.localPresences)).to.eql(['presence-2a']);
+          done();
+        });
+        next();
+      },
+      function(next) {
+        presence2a = connection2.getPresence('test-channel');
+        presence2a.create('presence-2a');
+        presence2a.subscribe(function(error) {
+          next(error);
+        });
+      }
+    ], errorHandler(done));
+  });
+
+  it('destroys old local presence but keeps new local presence when getting during destroy', function(done) {
+    presence2.create('presence-2');
+    var presence2a;
+
+    async.series([
+      presence2.subscribe.bind(presence2),
+      function(next) {
+        presence2.destroy(function() {
+          expect(presence2).to.equal(presence2a);
+          expect(Object.keys(presence2.localPresences)).to.eql(['presence-2a']);
+          done();
+        });
+        next();
+      },
+      function(next) {
+        presence2a = connection2.getPresence('test-channel');
+        presence2a.create('presence-2a');
+        presence2a.subscribe(function(error) {
+          next(error);
+        });
+      }
+    ], errorHandler(done));
+  });
+
+  it('throws if trying to create local presence when wanting destroy', function(done) {
+    presence2.destroy(errorHandler(done));
+    expect(function() {
+      presence2.create('presence-2');
+    }).to.throw('Presence is being destroyed');
+    done();
+  });
+
+  it('gets presence after destroy unsubscribe', function(done) {
+    var localPresence2 = presence2.create('presence-2');
+    var presence2a;
+
+    var flushLocalPresence2Destroy;
+    sinon.stub(localPresence2, 'destroy').callsFake(function(callback) {
+      flushLocalPresence2Destroy = callback;
+    });
+
+    async.series([
+      presence2.subscribe.bind(presence2),
+      function(next) {
+        presence2.destroy(function() {
+          expect(connection2.getPresence('test-channel')).to.equal(presence2a);
+          done();
+        });
+        next();
+      },
+      // Wait for the destroy unsubscribe callback to start, where we check
+      // _wantsDestroy for the first time
+      presence2.unsubscribe.bind(presence2),
+      function(next) {
+        presence2a = connection2.getPresence('test-channel');
+        presence2a.subscribe(function(error) {
+          next(error);
+        });
+        flushLocalPresence2Destroy();
+      }
+    ], errorHandler(done));
+  });
+
   it('requests existing presence from other subscribed clients when subscribing', function(done) {
     var localPresence1 = presence1.create('presence-1');
     async.series([