From ff359a73c67557f41bc14564bb734c2c80bc6446 Mon Sep 17 00:00:00 2001
From: David Glasser <glasser@meteor.com>
Date: Tue, 9 Oct 2012 17:01:12 -0700
Subject: [PATCH] Make sure that the client doesn't get confused by an unsub
 followed immediately by an identical sub.

Previously, it would fail to subscribe. This confused the meteor.currentUser
subscription in accounts_client.js. Reproduction:

  - Create user X with email (which sends a confirmation email)
  - Log out.
  - Log in as User Y in tab 1.
  - Follow the confirmation link in tab 2. This leaves you logged in
    as User Y.
  - Tab 1's localstorage poller notices that there's a new token and logs in
    with it.
  - After a successful login, Accounts._makeClientLoggedIn unsubs from
    meteor.currentUser and immediately resubs. It thinks there's already an
    existing sub, so it doesn't send the sub message and does immediately call
    the ready callback (which sets userLoaded() to true).
  - The unsub gets sent and the object in Meteor.users() gets depopulated.

Now Meteor.userLoaded() is true but Meteor.user() is empty.
---
 packages/livedata/livedata_connection.js      |  6 +++-
 .../livedata/livedata_connection_tests.js     | 31 ++++++++++++++++++-
 packages/meteor/timers.js                     |  7 ++++-
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/packages/livedata/livedata_connection.js b/packages/livedata/livedata_connection.js
index 32e7c754f6..ec662c8785 100644
--- a/packages/livedata/livedata_connection.js
+++ b/packages/livedata/livedata_connection.js
@@ -223,7 +223,11 @@ _.extend(Meteor._LivedataConnection.prototype, {
     if (args.length && typeof args[args.length - 1] === "function")
       var callback = args.pop();
 
-    var existing = self.subs.find({name: name, args: args}, {reactive: false}).fetch();
+    // Look for existing subs (ignore those with count=0, since they're going to
+    // get removed on the next time through the event loop).
+    var existing = self.subs.find(
+      {name: name, args: args, count: {$gt: 0}},
+      {reactive: false}).fetch();
 
     if (existing && existing[0]) {
       // already subbed, inc count.
diff --git a/packages/livedata/livedata_connection_tests.js b/packages/livedata/livedata_connection_tests.js
index 488e3a8c24..6e216e2d76 100644
--- a/packages/livedata/livedata_connection_tests.js
+++ b/packages/livedata/livedata_connection_tests.js
@@ -59,7 +59,7 @@ Tinytest.add("livedata stub - receive data", function (test) {
   test.isUndefined(conn.queued[coll_name]);
 });
 
-Tinytest.add("livedata stub - subscribe", function (test) {
+Tinytest.addAsync("livedata stub - subscribe", function (test, onComplete) {
   var stream = new Meteor._StubStream();
   var conn = newConnection(stream);
 
@@ -72,6 +72,7 @@ Tinytest.add("livedata stub - subscribe", function (test) {
   });
   test.isFalse(callback_fired);
 
+  test.length(stream.sent, 1);
   var message = JSON.parse(stream.sent.shift());
   var id = message.id;
   delete message.id;
@@ -80,6 +81,34 @@ Tinytest.add("livedata stub - subscribe", function (test) {
   // get the sub satisfied. callback fires.
   stream.receive({msg: 'data', 'subs': [id]});
   test.isTrue(callback_fired);
+
+  // This defers the actual unsub message, so we need to set a timeout
+  // to observe the message. We also test that we can resubscribe even
+  // before the unsub has been sent.
+  //
+  // Note: it would be perfectly fine for livedata_connection to send the unsub
+  // synchronously, so if this test fails just because we've made that change,
+  // that's OK! This is a regression test for a failure case where it *never*
+  // sent the unsub if there was a quick resub afterwards.
+  //
+  // XXX rewrite Meteor.defer to guarantee ordered execution so we don't have to
+  // use setTimeout
+  sub.stop();
+  conn.subscribe('my_data');
+
+  test.length(stream.sent, 1);
+  message = JSON.parse(stream.sent.shift());
+  var id2 = message.id;
+  test.notEqual(id, id2);
+  delete message.id;
+  test.equal(message, {msg: 'sub', name: 'my_data', params: []});
+
+  setTimeout(function() {
+    test.length(stream.sent, 1);
+    var message = JSON.parse(stream.sent.shift());
+    test.equal(message, {msg: 'unsub', id: id});
+    onComplete();
+  }, 10);
 });
 
 
diff --git a/packages/meteor/timers.js b/packages/meteor/timers.js
index a7353b1a77..bdaa636ef8 100644
--- a/packages/meteor/timers.js
+++ b/packages/meteor/timers.js
@@ -42,10 +42,15 @@ _.extend(Meteor, {
   },
 
   // won't be necessary once we clobber the global setTimeout
+  //
+  // XXX consider making this guarantee ordering of defer'd callbacks, like
+  // Meteor._atFlush or Node's nextTick (in practice). Then tests can do:
+  //    callSomethingThatDefersSomeWork();
+  //    Meteor.defer(expect(somethingThatValidatesThatTheWorkHappened));
   defer: function (f) {
     // Older Firefox will pass an argument to the setTimeout callback
     // function, indicating the "actual lateness." It's non-standard,
     // so for defer, standardize on not having it.
     Meteor.setTimeout(function () {f();}, 0);
   }
-});
\ No newline at end of file
+});