Merge pull request #124 from matrix-org/rav/limit_pagination

Avoid paginating forever in private rooms

lib/timeline-window.js

@@ -30,6 +30,13 @@ var DEBUG = false;
  */
 var debuglog = DEBUG ? console.log.bind(console) : function() {};
 
+/**
+ * the number of times we ask the server for more events before giving up
+ *
+ * @private
+ */
+var DEFAULT_PAGINATE_LOOP_LIMIT = 5;
+
 /**
  * Construct a TimelineWindow.
  *
@@ -179,10 +186,14 @@ TimelineWindow.prototype.canPaginate = function(direction) {
  *    even if there are fewer than 'size' of them, as we will just return those
  *    we already know about.)
  *
+ * @param {number} [requestLimit = 5] limit for the number of API requests we
+ *    should make.
+ *
  * @return {module:client.Promise} Resolves to a boolean which is true if more events
  *    were successfully retrieved.
  */
-TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
+TimelineWindow.prototype.paginate = function(direction, size, makeRequest,
+                                             requestLimit) {
     // Either wind back the message cap (if there are enough events in the
     // timeline to do so), or fire off a pagination request.
 
@@ -190,6 +201,10 @@ TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
         makeRequest = true;
     }
 
+    if (requestLimit === undefined) {
+        requestLimit = DEFAULT_PAGINATE_LOOP_LIMIT;
+    }
+
     var tl;
     if (direction == EventTimeline.BACKWARDS) {
         tl = this._start;
@@ -224,7 +239,9 @@ TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
         return q(true);
     }
 
-    if (!makeRequest) {
+    if (!makeRequest || requestLimit === 0) {
+        // todo: should we return something different to indicate that there
+        // might be more events out there, but we haven't found them yet?
         return q(false);
     }
 
@@ -256,7 +273,12 @@ TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
         // token. In particular, we want to know if we've actually hit the
         // start of the timeline, or if we just happened to know about all of
         // the events thanks to https://matrix.org/jira/browse/SYN-645.
-        return self.paginate(direction, size, true);
+        //
+        // On the other hand, we necessarily want to wait forever for the
+        // server to make its mind up about whether there are other events,
+        // because it gives a bad user experience
+        // (https://github.com/vector-im/vector-web/issues/1204).
+        return self.paginate(direction, size, true, requestLimit - 1);
     });
     tl.pendingPaginate = prom;
     return prom;

spec/unit/timeline-window.spec.js

@@ -418,5 +418,43 @@ describe("TimelineWindow", function() {
             }).catch(utils.failTest).done(done);
         });
 
+        it("should limit the number of unsuccessful pagination requests",
+        function(done) {
+            var timeline = createTimeline();
+            timeline.setPaginationToken("toktok", EventTimeline.FORWARDS);
+
+            var timelineWindow = createWindow(timeline, {windowLimit: 5});
+            var eventId = timeline.getEvents()[1].getId();
+
+            var paginateCount = 0;
+            client.paginateEventTimeline = function(timeline0, opts) {
+                expect(timeline0).toBe(timeline);
+                expect(opts.backwards).toBe(false);
+                expect(opts.limit).toEqual(2);
+                paginateCount += 1;
+                return q(true);
+            };
+
+            timelineWindow.load(eventId, 3).then(function() {
+                var expectedEvents = timeline.getEvents();
+                expect(timelineWindow.getEvents()).toEqual(expectedEvents);
+
+                expect(timelineWindow.canPaginate(EventTimeline.BACKWARDS))
+                    .toBe(false);
+                expect(timelineWindow.canPaginate(EventTimeline.FORWARDS))
+                    .toBe(true);
+                return timelineWindow.paginate(EventTimeline.FORWARDS, 2, true, 3);
+            }).then(function(success) {
+                expect(success).toBe(false);
+                expect(paginateCount).toEqual(3);
+                var expectedEvents = timeline.getEvents().slice(0, 3);
+                expect(timelineWindow.getEvents()).toEqual(expectedEvents);
+
+                expect(timelineWindow.canPaginate(EventTimeline.BACKWARDS))
+                    .toBe(false);
+                expect(timelineWindow.canPaginate(EventTimeline.FORWARDS))
+                    .toBe(true);
+            }).catch(utils.failTest).done(done);
+        });
     });
 });