From b903cef583f7aabb2007c865a1a39735b3eee4ab Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 24 Oct 2024 13:15:49 -0400 Subject: [PATCH] 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. Fixes: #945 Signed-off-by: Zixuan James Li --- lib/model/message_list.dart | 76 ++++++++++++++++++++++++----- test/model/message_list_test.dart | 79 ++++++++++++++++++++++++++++++- 2 files changed, 141 insertions(+), 14 deletions(-) diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index f47258a5bd..9847a591f6 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -1,6 +1,9 @@ +import 'dart:async'; + import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; +import '../api/backoff.dart'; import '../api/model/events.dart'; import '../api/model/model.dart'; import '../api/route/messages.dart'; @@ -89,9 +92,32 @@ mixin _MessageSequence { bool _haveOldest = false; /// Whether we are currently fetching the next batch of older messages. + /// + /// When this is true, [fetchOlder] is a no-op. + /// That method is called frequently by Flutter's scrolling logic, + /// and this field helps us avoid spamming the same request just to get + /// the same response each time. + /// + /// See also [fetchOlderCoolingDown]. bool get fetchingOlder => _fetchingOlder; bool _fetchingOlder = false; + /// Whether [fetchOlder] had a request error recently. + /// + /// When this is true, [fetchOlder] is a no-op. + /// That method is called frequently by Flutter's scrolling logic, + /// and this field mitigates spamming the same request and getting + /// the same error each time. + /// + /// "Recently" is decided by a [BackoffMachine] that resets + /// when a [fetchOlder] request succeeds. + /// + /// See also [fetchingOlder]. + bool get fetchOlderCoolingDown => _fetchOlderCoolingDown; + bool _fetchOlderCoolingDown = false; + + BackoffMachine? _fetchOlderCooldownBackoffMachine; + /// The parsed message contents, as a list parallel to [messages]. /// /// The i'th element is the result of parsing the i'th element of [messages]. @@ -107,7 +133,7 @@ mixin _MessageSequence { /// before, between, or after the messages. /// /// This information is completely derived from [messages] and - /// the flags [haveOldest] and [fetchingOlder]. + /// the flags [haveOldest], [fetchingOlder] and [fetchOlderCoolingDown]. /// It exists as an optimization, to memoize that computation. final QueueList items = QueueList(); @@ -241,6 +267,8 @@ mixin _MessageSequence { _fetched = false; _haveOldest = false; _fetchingOlder = false; + _fetchOlderCoolingDown = false; + _fetchOlderCooldownBackoffMachine = null; contents.clear(); items.clear(); } @@ -290,10 +318,12 @@ mixin _MessageSequence { void _updateEndMarkers() { assert(fetched); assert(!(haveOldest && fetchingOlder)); - final startMarker = switch ((fetchingOlder, haveOldest)) { - (true, _) => const MessageListLoadingItem(MessageListDirection.older), - (_, true) => const MessageListHistoryStartItem(), - (_, _) => null, + assert(!(fetchingOlder && fetchOlderCoolingDown)); + final startMarker = switch ((fetchingOlder, haveOldest, fetchOlderCoolingDown)) { + (true, _, _) => const MessageListLoadingItem(MessageListDirection.older), + (_, true, _) => const MessageListHistoryStartItem(), + (_, _, true) => const MessageListLoadingItem(MessageListDirection.older), + (_, _, _) => null, }; final hasStartMarker = switch (items.firstOrNull) { MessageListLoadingItem() => true, @@ -470,7 +500,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetchInitial() async { // TODO(#80): fetch from anchor firstUnread, instead of newest // TODO(#82): fetch from a given message ID as anchor - assert(!fetched && !haveOldest && !fetchingOlder); + assert(!fetched && !haveOldest && !fetchingOlder && !fetchOlderCoolingDown); assert(messages.isEmpty && contents.isEmpty); // TODO schedule all this in another isolate final generation = this.generation; @@ -498,20 +528,28 @@ class MessageListView with ChangeNotifier, _MessageSequence { Future fetchOlder() async { if (haveOldest) return; if (fetchingOlder) return; + if (fetchOlderCoolingDown) return; assert(fetched); assert(messages.isNotEmpty); _fetchingOlder = true; _updateEndMarkers(); notifyListeners(); final generation = this.generation; + bool hasFetchError = false; try { - final result = await getMessages(store.connection, - narrow: narrow.apiEncode(), - anchor: NumericAnchor(messages[0].id), - includeAnchor: false, - numBefore: kMessageListFetchBatchSize, - numAfter: 0, - ); + final GetMessagesResult result; + try { + result = await getMessages(store.connection, + narrow: narrow.apiEncode(), + anchor: NumericAnchor(messages[0].id), + includeAnchor: false, + numBefore: kMessageListFetchBatchSize, + numAfter: 0, + ); + } catch (e) { + hasFetchError = true; + rethrow; + } if (this.generation > generation) return; if (result.messages.isNotEmpty @@ -529,9 +567,21 @@ class MessageListView with ChangeNotifier, _MessageSequence { _insertAllMessages(0, fetchedMessages); _haveOldest = result.foundOldest; + _fetchOlderCooldownBackoffMachine = null; } finally { if (this.generation == generation) { _fetchingOlder = false; + if (hasFetchError) { + assert(!fetchOlderCoolingDown); + _fetchOlderCoolingDown = true; + unawaited((_fetchOlderCooldownBackoffMachine ??= BackoffMachine()) + .wait().then((_) { + if (this.generation != generation) return; + _fetchOlderCoolingDown = false; + _updateEndMarkers(); + notifyListeners(); + })); + } _updateEndMarkers(); notifyListeners(); } diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 9215e497e2..42e23eac64 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -3,6 +3,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; +import 'package:zulip/api/exception.dart'; import 'package:zulip/api/model/events.dart'; import 'package:zulip/api/model/model.dart'; import 'package:zulip/api/model/narrow.dart'; @@ -238,6 +239,42 @@ void main() { ..messages.length.equals(30); }); + test('fetchOlder nop during backoff', () => awaitFakeAsync((async) async { + final olderMessages = List.generate(5, (i) => eg.streamMessage()); + final initialMessages = List.generate(5, (i) => eg.streamMessage()); + await prepare(narrow: const CombinedFeedNarrow()); + await prepareMessages(foundOldest: false, messages: initialMessages); + + connection.prepare(httpStatus: 400, json: { + 'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'}); + check(async.pendingTimers).isEmpty(); + await check(model.fetchOlder()).throws(); + checkNotified(count: 2); + check(model).fetchOlderCoolingDown.isTrue(); + + // Drop irrelevant requests with without checking, + // as we are only interested in verifying if any request was sent. + connection.takeRequests(); + + await model.fetchOlder(); + checkNotNotified(); + check(model).fetchOlderCoolingDown.isTrue(); + check(model).fetchingOlder.isFalse(); + check(connection.lastRequest).isNull(); + + // The first backoff is expected to be short enough to complete. + async.elapse(const Duration(seconds: 1)); + check(model).fetchOlderCoolingDown.isFalse(); + checkNotifiedOnce(); + check(connection.lastRequest).isNull(); + + connection.prepare(json: olderResult( + anchor: 1000, foundOldest: false, messages: olderMessages).toJson()); + await model.fetchOlder(); + checkNotified(count: 2); + check(connection.lastRequest).isNotNull(); + })); + test('fetchOlder handles servers not understanding includeAnchor', () async { const narrow = CombinedFeedNarrow(); await prepare(narrow: narrow); @@ -985,6 +1022,45 @@ void main() { checkNotifiedOnce(); })); + test('fetchOlder backoff start, _reset, fetchOlder backoff ends, move fetch finishes', () => awaitFakeAsync((async) async { + await prepareNarrow(narrow, initialMessages); + + connection.prepare(httpStatus: 400, json: { + 'result': 'error', 'code': 'BAD_REQUEST', 'msg': 'Bad request'}); + await check(model.fetchOlder()).throws(); + final backoffTimer = async.pendingTimers.single; + check(model).fetchOlderCoolingDown.isTrue(); + checkHasMessages(initialMessages); + checkNotified(count: 2); + + connection.prepare(delay: const Duration(seconds: 2), json: newestResult( + foundOldest: false, + messages: initialMessages + movedMessages, + ).toJson()); + check(model).fetched.isTrue(); + await store.handleEvent(eg.updateMessageEventMoveTo( + origTopic: movedMessages[0].topic, + origStreamId: otherStream.streamId, + newMessages: movedMessages, + )); + // check that _reset was caleed + check(model).fetched.isFalse(); + check(model).fetchOlderCoolingDown.isFalse(); + check(async.pendingTimers).contains(backoffTimer); + checkHasMessages([]); + checkNotifiedOnce(); + + // The first backoff is expected to be short enough to complete. + async.elapse(const Duration(seconds: 1)); + check(model).fetchOlderCoolingDown.isFalse(); + check(async.pendingTimers).not((x) => x.contains(backoffTimer)); + checkNotNotified(); + + async.elapse(const Duration(seconds: 1)); + checkHasMessages(initialMessages + movedMessages); + checkNotifiedOnce(); + })); + test('fetchOlder, _reset, move fetch finishes, fetchOlder returns', () => awaitFakeAsync((async) async { await prepareNarrow(narrow, initialMessages); @@ -1793,7 +1869,7 @@ void checkInvariants(MessageListView model) { if (model.haveOldest) { check(model.items[i++]).isA(); } - if (model.fetchingOlder) { + if (model.fetchingOlder || model.fetchOlderCoolingDown) { check(model.items[i++]).isA(); } for (int j = 0; j < model.messages.length; j++) { @@ -1849,4 +1925,5 @@ extension MessageListViewChecks on Subject { Subject get fetched => has((x) => x.fetched, 'fetched'); Subject get haveOldest => has((x) => x.haveOldest, 'haveOldest'); Subject get fetchingOlder => has((x) => x.fetchingOlder, 'fetchingOlder'); + Subject get fetchOlderCoolingDown => has((x) => x.fetchOlderCoolingDown, 'fetchOlderCoolingDown'); }