Skip to content

Commit 62e334c

Browse files
committed
msglist: Throttle fetchOlder retries
This approach is different from how a BackoffMachine is typically used, because the message list doesn't send and retry requests in a loop; its caller retries rapidly on scroll changes, and we want to ignore the excessive requests. The test drops irrelevant requests with `connection.takeRequests` without checking, as we are only interested in verifying that no request was sent. Fixes: #945 Signed-off-by: Zixuan James Li <[email protected]>
1 parent cdce570 commit 62e334c

File tree

2 files changed

+57
-7
lines changed

2 files changed

+57
-7
lines changed

lib/model/message_list.dart

+25-7
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1+
import 'dart:async';
2+
13
import 'package:collection/collection.dart';
24
import 'package:flutter/foundation.dart';
35

6+
import '../api/backoff.dart';
47
import '../api/model/events.dart';
58
import '../api/model/model.dart';
69
import '../api/route/messages.dart';
@@ -493,24 +496,38 @@ class MessageListView with ChangeNotifier, _MessageSequence {
493496
notifyListeners();
494497
}
495498

499+
bool _waitBeforeRetry = false;
500+
BackoffMachine? _backoffMachine;
501+
496502
/// Fetch the next batch of older messages, if applicable.
497503
Future<void> fetchOlder() async {
498504
if (haveOldest) return;
499505
if (fetchingOlder) return;
506+
if (_waitBeforeRetry) return;
500507
assert(fetched);
501508
assert(messages.isNotEmpty);
502509
_fetchingOlder = true;
503510
_updateEndMarkers();
504511
notifyListeners();
505512
final generation = this.generation;
506513
try {
507-
final result = await getMessages(store.connection,
508-
narrow: narrow.apiEncode(),
509-
anchor: NumericAnchor(messages[0].id),
510-
includeAnchor: false,
511-
numBefore: kMessageListFetchBatchSize,
512-
numAfter: 0,
513-
);
514+
final GetMessagesResult result;
515+
try {
516+
result = await getMessages(store.connection,
517+
narrow: narrow.apiEncode(),
518+
anchor: NumericAnchor(messages[0].id),
519+
includeAnchor: false,
520+
numBefore: kMessageListFetchBatchSize,
521+
numAfter: 0,
522+
);
523+
} catch (e) {
524+
assert(!_waitBeforeRetry);
525+
_waitBeforeRetry = true;
526+
unawaited((_backoffMachine ??= BackoffMachine()).wait().then((_) {
527+
_waitBeforeRetry = false;
528+
}));
529+
rethrow;
530+
}
514531
if (this.generation > generation) return;
515532

516533
if (result.messages.isNotEmpty
@@ -528,6 +545,7 @@ class MessageListView with ChangeNotifier, _MessageSequence {
528545

529546
_insertAllMessages(0, fetchedMessages);
530547
_haveOldest = result.foundOldest;
548+
_backoffMachine = null;
531549
} finally {
532550
if (this.generation == generation) {
533551
_fetchingOlder = false;

test/model/message_list_test.dart

+32
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import 'dart:convert';
33
import 'package:checks/checks.dart';
44
import 'package:http/http.dart' as http;
55
import 'package:test/scaffolding.dart';
6+
import 'package:zulip/api/exception.dart';
67
import 'package:zulip/api/model/events.dart';
78
import 'package:zulip/api/model/model.dart';
89
import 'package:zulip/api/model/narrow.dart';
@@ -236,6 +237,37 @@ void main() {
236237
..messages.length.equals(30);
237238
});
238239

240+
test('fetchOlder nop during backoff', () => awaitFakeAsync((async) async {
241+
final olderMessages = List.generate(5, (i) => eg.streamMessage());
242+
final initialMessages = List.generate(5, (i) => eg.streamMessage());
243+
await prepare(narrow: const CombinedFeedNarrow());
244+
await prepareMessages(foundOldest: false, messages: initialMessages);
245+
check(model).messages.length.equals(5);
246+
247+
connection.prepare(httpStatus: 400, json: {
248+
'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'});
249+
check(async.pendingTimers).isEmpty();
250+
await check(model.fetchOlder()).throws<ZulipApiException>();
251+
// The backoff timer has started.
252+
check(async.pendingTimers).single;
253+
checkNotified(count: 2);
254+
check(model).messages.length.equals(5);
255+
256+
connection.takeRequests();
257+
await model.fetchOlder();
258+
check(connection.lastRequest).isNull();
259+
check(async.pendingTimers).single;
260+
checkNotNotified();
261+
check(model).messages.length.equals(5);
262+
263+
async.flushTimers();
264+
connection.prepare(json: olderResult(
265+
anchor: 1000, foundOldest: false, messages: olderMessages).toJson());
266+
await model.fetchOlder();
267+
checkNotified(count: 2);
268+
check(model).messages.length.equals(10);
269+
}));
270+
239271
test('fetchOlder handles servers not understanding includeAnchor', () async {
240272
const narrow = CombinedFeedNarrow();
241273
await prepare(narrow: narrow);

0 commit comments

Comments
 (0)