From ebebf9abef719276f2ab617302fe1e27b5bfb9cc Mon Sep 17 00:00:00 2001
From: Chris Mather <mather.chris@gmail.com>
Date: Mon, 18 Aug 2014 10:56:00 -0700
Subject: [PATCH] throw an error on infinite waitlist invalidation loops.

Users are finding themselves in a situation of infinite invalidation
loops. This can happen if you call this.ready() and then subsequently
add functions to the list which change the state in the same computation
tree.

Fix:
When you call wait(...) see whether we already have a dependency on the
current computation or any of its ancestors and if so throw a helpful
error message. For example:

var list = new WaitList;
Deps.autorun(function () {
 list.ready();

 Deps.autorun(function () {
   list.wait(...); // throw error
 });
});

Issue #742

cc @tmeasday, @DerMambo
---
 lib/client/wait_list.js  | 76 ++++++++++++++++++++++++++++++++++------
 test/client/wait_list.js | 25 ++++---------
 2 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/lib/client/wait_list.js b/lib/client/wait_list.js
index a1ac6648..4622e32a 100644
--- a/lib/client/wait_list.js
+++ b/lib/client/wait_list.js
@@ -1,11 +1,69 @@
+/*****************************************************************************/
+/* Imports */
+/*****************************************************************************/
+var assert = Iron.utils.assert;
+
+/*****************************************************************************/
+/* Private */
+/*****************************************************************************/
+
+/**
+ * Returns an object of computation ids starting with
+ * the current computation and including all ancestor
+ * computations. The data structure is an object
+ * so we can index by id and do quick checks.
+ */
+var parentComputations = function () {
+  var list = {};
+  var c = Deps.currentComputation;
+
+  while (c) {
+    list[String(c._id)] = true;
+    c = c._parent;
+  }
+
+  return list;
+};
+
+/**
+ * Check whether the user has called ready() and then called wait(). This
+ * can cause a condition that can be simplified to this:
+ *
+ * dep = new Deps.Dependency;
+ *
+ * Deps.autorun(function () {
+ *   dep.depend();
+ *   dep.changed();
+ * });
+ */
+var assertNoInvalidationLoop = function (dependency) {
+  var parentComps = parentComputations();
+  var depCompIds = Object.keys(dependency._dependentsById);
+
+  depCompIds.forEach(function (id) {
+    console.log('id: ' + id);
+    assert(!parentComps[id], "\n\n\
+You called wait() after calling ready() inside the same computation tree.\
+\n\n\
+You can fix this problem in two possible ways:\n\n\
+1) Put all of your wait() calls before any ready() calls.\n\
+2) Put your ready() call in its own computation with Deps.autorun."
+    );
+  });
+};
+
+
+/*****************************************************************************/
+/* WaitList */
+/*****************************************************************************/
 /**
- * A WaitList evaluates a series of functions, each in its own computation. The
- * list is ready() when all of the functions return true. This list is not ready
- * (i.e. this.ready() === false) if at least one function returns false.
+ * A WaitList tracks a list of reactive functions, each in its own computation.
+ * The list is ready() when all of the functions return true. This list is not
+ * ready (i.e. this.ready() === false) if at least one function returns false.
  *
  * You add functions by calling the wait(fn) method. Each function is run its
  * own computation. The ready() method is a reactive method but only calls the
- * deps changed function if the overall value of the list changes from true to
+ * deps changed function if the overall state of the list changes from true to
  * false or from false to true.
  */
 WaitList = function () {
@@ -22,6 +80,8 @@ WaitList.prototype.wait = function (fn) {
 
   var activeComp = Deps.currentComputation;
 
+  assertNoInvalidationLoop(self._readyDep);
+
   // break with parent computation and grab the new comp
   Deps.nonreactive(function () {
 
@@ -46,14 +106,10 @@ WaitList.prototype.wait = function (fn) {
 
       cachedResult = result;
 
-      var change = function () {
-        Deps.afterFlush(function () { self._readyDep.changed(); });
-      };
-
       if (oldNotReadyCount === 0 && self._notReadyCount > 0)
-        change();
+        self._readyDep.changed();
       else if (oldNotReadyCount > 0 && self._notReadyCount === 0)
-        change();
+        self._readyDep.changed();
     });
 
     self._comps.push(comp);
diff --git a/test/client/wait_list.js b/test/client/wait_list.js
index f50ce3d7..3e38d154 100644
--- a/test/client/wait_list.js
+++ b/test/client/wait_list.js
@@ -64,26 +64,15 @@ Tinytest.add('WaitList - infinite invalidation loop', function (test) {
     // because there's no items in the list
     ready = list.ready();
     
-    // now add an item to the list that invalidates
-    // the outer computation because the list ready
-    // state is now: false
-    // goal is to just rerun the outer function once, not
-    // infinitely
-    list.wait(function() {
-      return handle.ready();
+    
+    // this should throw an exception because it would cause an infinite
+    // loop.
+    test.throws(function () {
+      list.wait(function() {
+        return handle.ready();
+      });
     });
   });
-  
-  Deps.flush();
-  test.equal(ready, false);
-  if (times > 2)
-    return test.fail({message: "Autorun ran too many times"});
-  
-  handle.set(true);
-  Deps.flush();
-  test.equal(ready, true);
-  if (times > 3)
-    return test.fail({message: "Autorun ran too many times"});
 });
 
 Tinytest.add('WaitList - ready state must always be accurate', function (test) {