diff --git a/lib/api/core.dart b/lib/api/core.dart index 200140fff..a217c6de7 100644 --- a/lib/api/core.dart +++ b/lib/api/core.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -179,16 +180,7 @@ class ApiConnection { try { response = await _client.send(request); } catch (e) { - final String message; - if (e is http.ClientException) { - message = e.message; - } else if (e is TlsException) { - message = e.message; - } else { - final zulipLocalizations = GlobalLocalizations.zulipLocalizations; - message = zulipLocalizations.errorNetworkRequestFailed; - } - throw NetworkException(routeName: routeName, cause: e, message: message); + _throwNetworkException(routeName, e); } final int httpStatus = response.statusCode; @@ -198,6 +190,9 @@ class ApiConnection { // while the response is still being downloaded, improving latency. final jsonStream = jsonUtf8Decoder.bind(response.stream); json = await jsonStream.single as Map?; + } on http.RequestAbortedException catch (e) { + // The timeout in [_withTimeout] fired while we were reading the response. + _throwNetworkException(routeName, e); } catch (e) { // We'll throw something below, seeing `json` is null. } @@ -223,12 +218,38 @@ class ApiConnection { _isOpen = false; } + /// Make a GET request to the given path with the given params. + /// + /// If [timeout] is non-null and the request hasn't completed within it, + /// including reading the response body, + /// then the request is aborted, tearing down the connection, + /// and this throws a [NetworkException] + /// with kind [NetworkExceptionKind.connectionFailed]. Future get(String routeName, T Function(Map) fromJson, - String path, Map? params) async { + String path, Map? params, {Duration? timeout}) async { final url = realmUrl.replace( path: "/api/v1/$path", queryParameters: encodeParameters(params)); - final request = http.Request('GET', url); - return send(routeName, fromJson, request); + if (timeout == null) { + return send(routeName, fromJson, http.Request('GET', url)); + } + return _withTimeout(timeout, (abortTrigger) => send(routeName, fromJson, + http.AbortableRequest('GET', url, abortTrigger: abortTrigger))); + } + + /// Run [body] with a trigger that fires after [timeout], + /// for constructing an [http.Abortable] request. + /// + /// The timer is canceled when the returned future completes, + /// so a request that finishes in time leaves no timer behind. + Future _withTimeout(Duration timeout, + Future Function(Future abortTrigger) body) async { + final abortCompleter = Completer(); + final abortTimer = Timer(timeout, abortCompleter.complete); + try { + return await body(abortCompleter.future); + } finally { + abortTimer.cancel(); + } } Future post(String routeName, T Function(Map) fromJson, @@ -280,6 +301,29 @@ class ApiConnection { } } +/// Throw a [NetworkException] wrapping the given exception +/// from the underlying HTTP client. +Never _throwNetworkException(String routeName, Object cause) { + final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + final (NetworkExceptionKind kind, String message) = switch (cause) { + // Our own timeout, from [ApiConnection._withTimeout]. Skip the + // exception's message: it names a package-internal mechanism + // ("Request aborted by `abortTrigger`"), which wouldn't mean much to a user. + http.RequestAbortedException() => + (.connectionFailed, zulipLocalizations.errorNetworkRequestFailed), + // A wrapped SocketException, like package:http's IOClient throws + // for a connection failure. + SocketException() && http.ClientException(:final message) => + (.connectionFailed, message), + SocketException() => (.connectionFailed, zulipLocalizations.errorNetworkRequestFailed), + http.ClientException(:final message) => (.other, message), + TlsException(:final message) => (.other, message), + _ => (.other, zulipLocalizations.errorNetworkRequestFailed), + }; + throw NetworkException(routeName: routeName, + kind: kind, cause: cause, message: message); +} + ApiRequestException _makeApiException(String routeName, int httpStatus, Map? json) { assert(httpStatus != 200 || json == null); if (400 <= httpStatus && httpStatus <= 499) { diff --git a/lib/api/exception.dart b/lib/api/exception.dart index d495ec9ff..6b67c25ba 100644 --- a/lib/api/exception.dart +++ b/lib/api/exception.dart @@ -28,19 +28,58 @@ sealed class ApiRequestException implements Exception { String toString() => message; } +/// What sort of underlying problem caused a [NetworkException]. +/// +/// The classification is made in [ApiConnection.send], +/// which is the one place that sees the raw exception +/// from the underlying HTTP client. +/// Consumers should switch on this +/// rather than inspecting [NetworkException.cause], +/// because the cause's concrete type depends on +/// which HTTP client implementation is in use. +enum NetworkExceptionKind { + /// The connection couldn't be established, or was lost, + /// below the level of any HTTP response. + /// + /// This includes a request timing out, + /// which we treat as the connection having died + /// even if it still looks open. + /// + /// This is routine rather than a sign of a bug: + /// it's what happens when the device is offline, + /// and commonly when the app returns from sleep + /// or the device switches networks. + connectionFailed, + + /// Any other network-level failure. + other, +} + /// A network-level error that prevented even getting an HTTP response /// to some Zulip API network request. /// /// This is the antonym of [HttpException]. class NetworkException extends ApiRequestException { + /// What sort of problem this is. + final NetworkExceptionKind kind; + /// The exception describing the underlying error. /// /// This can be any exception value that [http.Client.send] throws. /// Ideally that would always be an [http.ClientException], /// but empirically it can be [TlsException] and possibly others. + /// + /// The concrete type depends on which HTTP client implementation + /// the app is using, so prefer [kind] for making decisions; + /// this is most useful for logging. final Object cause; - NetworkException({required super.routeName, required super.message, required this.cause}); + NetworkException({ + required super.routeName, + required super.message, + required this.kind, + required this.cause, + }); @override String toString() { diff --git a/lib/api/model/initial_snapshot.dart b/lib/api/model/initial_snapshot.dart index 1709d007b..ea786bdfa 100644 --- a/lib/api/model/initial_snapshot.dart +++ b/lib/api/model/initial_snapshot.dart @@ -131,6 +131,8 @@ class InitialSnapshot { final Uri serverEmojiDataUrl; + final int eventQueueLongpollTimeoutSeconds; + final int? realmModerationRequestChannelId; // TODO(server-10) final String? realmEmptyTopicDisplayName; // TODO(server-10) @@ -216,6 +218,7 @@ class InitialSnapshot { required this.maxFileUploadSizeMib, required this.serverThumbnailFormats, required this.serverEmojiDataUrl, + required this.eventQueueLongpollTimeoutSeconds, required this.realmModerationRequestChannelId, required this.realmEmptyTopicDisplayName, required this.realmUsers, diff --git a/lib/api/model/initial_snapshot.g.dart b/lib/api/model/initial_snapshot.g.dart index 8ecc74a76..ae4eb657d 100644 --- a/lib/api/model/initial_snapshot.g.dart +++ b/lib/api/model/initial_snapshot.g.dart @@ -142,6 +142,8 @@ InitialSnapshot _$InitialSnapshotFromJson( .toList() ?? [], serverEmojiDataUrl: Uri.parse(json['server_emoji_data_url'] as String), + eventQueueLongpollTimeoutSeconds: + (json['event_queue_longpoll_timeout_seconds'] as num).toInt(), realmModerationRequestChannelId: (json['realm_moderation_request_channel_id'] as num?)?.toInt(), realmEmptyTopicDisplayName: json['realm_empty_topic_display_name'] as String?, @@ -226,6 +228,8 @@ Map _$InitialSnapshotToJson( 'max_file_upload_size_mib': instance.maxFileUploadSizeMib, 'server_thumbnail_formats': instance.serverThumbnailFormats, 'server_emoji_data_url': instance.serverEmojiDataUrl.toString(), + 'event_queue_longpoll_timeout_seconds': + instance.eventQueueLongpollTimeoutSeconds, 'realm_moderation_request_channel_id': instance.realmModerationRequestChannelId, 'realm_empty_topic_display_name': instance.realmEmptyTopicDisplayName, diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart index df12082fc..feb55749d 100644 --- a/lib/api/route/events.dart +++ b/lib/api/route/events.dart @@ -70,14 +70,20 @@ enum _IdleQueueTimeout { } /// https://zulip.com/api/get-events +/// +/// Long-lived clients should pass [timeout], +/// using the value the server recommends +/// in [InitialSnapshot.eventQueueLongpollTimeoutSeconds]; +/// see the discussion in the API docs for this endpoint. Future getEvents(ApiConnection connection, { required String queueId, int? lastEventId, bool? dontBlock, + Duration? timeout, }) { return connection.get('getEvents', GetEventsResult.fromJson, 'events', { 'queue_id': RawParameter(queueId), 'last_event_id': ?lastEventId, 'dont_block': ?dontBlock, - }); + }, timeout: timeout); } @JsonSerializable(fieldRename: FieldRename.snake) diff --git a/lib/model/realm.dart b/lib/model/realm.dart index cffb3095f..3d1f271ee 100644 --- a/lib/model/realm.dart +++ b/lib/model/realm.dart @@ -20,6 +20,9 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore { //|////////////////////////////////////////////////////////////// // Server settings, explicitly so named. + Duration get eventQueueLongpollTimeout => Duration(seconds: eventQueueLongpollTimeoutSeconds); + int get eventQueueLongpollTimeoutSeconds; + Duration get serverPresencePingInterval => Duration(seconds: serverPresencePingIntervalSeconds); int get serverPresencePingIntervalSeconds; Duration get serverPresenceOfflineThreshold => Duration(seconds: serverPresenceOfflineThresholdSeconds); @@ -171,6 +174,8 @@ mixin ProxyRealmStore on RealmStore { @protected RealmStore get realmStore; + @override + int get eventQueueLongpollTimeoutSeconds => realmStore.eventQueueLongpollTimeoutSeconds; @override int get serverPresencePingIntervalSeconds => realmStore.serverPresencePingIntervalSeconds; @override @@ -255,6 +260,7 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { }) : _selfUserRole = selfUser.role, _selfUserDateJoined = selfUser.dateJoined, + eventQueueLongpollTimeoutSeconds = initialSnapshot.eventQueueLongpollTimeoutSeconds, serverPresencePingIntervalSeconds = initialSnapshot.serverPresencePingIntervalSeconds, serverPresenceOfflineThresholdSeconds = initialSnapshot.serverPresenceOfflineThresholdSeconds, serverTypingStartedExpiryPeriodMilliseconds = initialSnapshot.serverTypingStartedExpiryPeriodMilliseconds, @@ -401,6 +407,9 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore { /// See also [_selfUserRole]. final String _selfUserDateJoined; + @override + final int eventQueueLongpollTimeoutSeconds; + @override final int serverPresencePingIntervalSeconds; @override diff --git a/lib/model/store.dart b/lib/model/store.dart index 5b554bf64..cd95dba0e 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -1504,8 +1504,8 @@ class UpdateMachine { // Print stack trace in its own log entry; log entries are truncated // at 1 kiB (at least on Android), and stack can be longer than that. assert(debugLog('Stack:\n$stackTrace')); - if (e case NetworkException(cause: SocketException())) { - // A [SocketException] is common when the device is asleep. + if (e case NetworkException(kind: .connectionFailed)) { + // A failed connection is common when the device is asleep. } else { // TODO: When the error seems transient, do keep retrying but // don't spam this feedback. @@ -1638,7 +1638,10 @@ class UpdateMachine { // ask the server to tell us immediately that it's working again, // rather than waiting for an event, which could take up to a minute // in the case of a heartbeat event. See #979. - dontBlock: store.isRecoveringEventStream ? true : null); + dontBlock: store.isRecoveringEventStream ? true : null, + // If the request outlives this, assume the connection is dead + // even if it still looks open; give up on it and retry. See #514. + timeout: store.eventQueueLongpollTimeout); if (_disposed) return; } catch (e, stackTrace) { if (_disposed) return; @@ -1741,8 +1744,8 @@ class UpdateMachine { bool shouldReportToUser; switch (error) { - case NetworkException(cause: SocketException()): - // A [SocketException] is common when the app returns from sleep. + case NetworkException(kind: .connectionFailed): + // A failed connection is common when the app returns from sleep. shouldReportToUser = false; case NetworkException(): diff --git a/test/api/core_test.dart b/test/api/core_test.dart index 903f35eef..e7861eaf8 100644 --- a/test/api/core_test.dart +++ b/test/api/core_test.dart @@ -10,6 +10,7 @@ import 'package:zulip/api/exception.dart'; import 'package:zulip/model/binding.dart'; import 'package:zulip/model/localizations.dart'; +import '../fake_async.dart'; import '../model/binding.dart'; import '../stdlib_checks.dart'; import '../test_async.dart'; @@ -283,17 +284,65 @@ void main() { } final zulipLocalizations = GlobalLocalizations.zulipLocalizations; + checkRequest(const SocketException('Oops'), (it) => it + ..kind.equals(.connectionFailed) + ..message.equals(zulipLocalizations.errorNetworkRequestFailed) + ..asString.equals( + 'NetworkException: Network request failed (SocketException: Oops)')); + // What `IOClient` actually throws when the connection fails: + // a ClientException that also implements SocketException. + checkRequest(ClientSocketException('Oops'), (it) => it + ..kind.equals(.connectionFailed) + ..message.equals('Oops')); checkRequest(http.ClientException('Oops'), (it) => it + ..kind.equals(.other) ..message.equals('Oops') ..asString.equals('NetworkException: Oops (ClientException: Oops)')); checkRequest(const TlsException('Oops'), (it) => it + ..kind.equals(.other) ..message.equals('Oops') ..asString.equals('NetworkException: Oops (TlsException: Oops)')); checkRequest((foo: 'bar'), (it) => it + ..kind.equals(.other) ..message.equals(zulipLocalizations.errorNetworkRequestFailed) ..asString.equals('NetworkException: Network request failed ((foo: bar))')); }); + test('API request timeout', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(delay: const Duration(seconds: 300), json: {}); + await check(connection.get(kExampleRouteName, (json) => json, + 'example/route', {}, timeout: const Duration(seconds: 90))) + .throws((it) => it + ..routeName.equals(kExampleRouteName) + ..kind.equals(.connectionFailed) + ..cause.isA()); + }); + })); + + test('API request timeout while reading response body', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(bodyDelay: const Duration(seconds: 300), json: {}); + await check(connection.get(kExampleRouteName, (json) => json, + 'example/route', {}, timeout: const Duration(seconds: 90))) + .throws((it) => it + ..routeName.equals(kExampleRouteName) + ..kind.equals(.connectionFailed) + ..cause.isA()); + }); + })); + + test('no API request timeout when response is in time', () => awaitFakeAsync((async) async { + await FakeApiConnection.with_((connection) async { + connection.prepare(delay: const Duration(seconds: 30), json: {'x': 3}); + final result = await connection.get(kExampleRouteName, (json) => json['x'], + 'example/route', {}, timeout: const Duration(seconds: 90)); + check(result).equals(3); + // The timeout's timer was canceled, not left to linger. + check(async.pendingTimers).isEmpty(); + }); + })); + test('API 4xx errors, well formed', () async { Future checkRequest({ int httpStatus = 400, @@ -491,6 +540,24 @@ class DistinctiveError extends Error { String toString() => message; } +/// Like the private `_ClientSocketException` +/// that `package:http`'s `IOClient` actually throws +/// when the underlying `HttpClient` throws a [SocketException]: +/// an [http.ClientException] that also implements [SocketException]. +class ClientSocketException extends http.ClientException + implements SocketException { + ClientSocketException(super.message, [super.uri]); + + @override + InternetAddress? get address => null; + + @override + OSError? get osError => null; + + @override + int? get port => null; +} + Future tryRequest({ Object? exception, int? httpStatus, diff --git a/test/api/exception_checks.dart b/test/api/exception_checks.dart index 6f7f07daf..3a2ab18a6 100644 --- a/test/api/exception_checks.dart +++ b/test/api/exception_checks.dart @@ -14,6 +14,7 @@ extension ZulipApiExceptionChecks on Subject { } extension NetworkExceptionChecks on Subject { + Subject get kind => has((e) => e.kind, 'kind'); Subject get cause => has((e) => e.cause, 'cause'); } diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 2382ab859..3b9b193a6 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -1,3 +1,4 @@ +import 'dart:async'; import 'dart:collection'; import 'dart:convert'; @@ -25,8 +26,14 @@ class _PreparedException extends _PreparedResponse { class _PreparedSuccess extends _PreparedResponse { final int httpStatus; final List bytes; - - _PreparedSuccess({super.delay, required this.httpStatus, required this.bytes}); + final Duration bodyDelay; + + _PreparedSuccess({ + super.delay, + required this.httpStatus, + required this.bytes, + this.bodyDelay = Duration.zero, + }); } /// An [http.Client] that accepts and replays canned responses, for testing. @@ -54,17 +61,24 @@ class FakeHttpClient extends http.BaseClient { /// /// If `exception` is non-null, then `httpStatus`, `body`, and `json` must /// all be null, and the next request will throw the given exception. + /// + /// In each case, the next request will complete a duration of `delay` + /// after being started. + /// On success, the response body arrives a further `bodyDelay` + /// after the response's headers. void prepare({ Object? exception, int? httpStatus, Map? json, String? body, Duration delay = Duration.zero, + Duration bodyDelay = Duration.zero, }) { // TODO: Prevent a source of bugs by ensuring that there are no outstanding // prepared responses when the test ends. if (exception != null) { - assert(httpStatus == null && json == null && body == null); + assert(httpStatus == null && json == null && body == null + && bodyDelay == Duration.zero); _preparedResponses.addLast(_PreparedException(exception: exception, delay: delay)); } else { assert((json == null) || (body == null)); @@ -77,6 +91,7 @@ class FakeHttpClient extends http.BaseClient { httpStatus: httpStatus ?? 200, bytes: utf8.encode(resolvedBody), delay: delay, + bodyDelay: bodyDelay, )); } } @@ -100,16 +115,50 @@ class FakeHttpClient extends http.BaseClient { } final response = _preparedResponses.removeFirst(); + final abortTrigger = + (request is http.Abortable) ? request.abortTrigger : null; + final http.StreamedResponse Function() computation; switch (response) { case _PreparedException(:var exception): computation = () => throw exception; - case _PreparedSuccess(:var bytes, :var httpStatus): - final byteStream = http.ByteStream.fromBytes(bytes); + case _PreparedSuccess(:var bytes, :var httpStatus, :var bodyDelay): computation = () => http.StreamedResponse( - byteStream, httpStatus, request: request); + _bodyStream(request, bytes: bytes, bodyDelay: bodyDelay, + abortTrigger: abortTrigger), + httpStatus, request: request); + } + final result = Future.delayed(response.delay, computation); + if (abortTrigger == null) return result; + // Mimic [IOClient]: if the trigger fires before the response headers + // are delivered, [send]'s future throws instead. + return Future.any([ + result, + abortTrigger.then((_) => throw http.RequestAbortedException(request.url)), + ]); + } + + /// The response body, mimicking [IOClient]'s abort behavior: + /// if [abortTrigger] fires before the body has been delivered, + /// inject an [http.RequestAbortedException] and close the stream. + http.ByteStream _bodyStream(http.BaseRequest request, { + required List bytes, + required Duration bodyDelay, + required Future? abortTrigger, + }) { + if (abortTrigger == null && bodyDelay == Duration.zero) { + return http.ByteStream.fromBytes(bytes); } - return Future.delayed(response.delay, computation); + final controller = StreamController>(); + Timer(bodyDelay, () { + if (controller.isClosed) return; + controller..add(bytes)..close(); + }); + abortTrigger?.whenComplete(() { + if (controller.isClosed) return; + controller..addError(http.RequestAbortedException(request.url))..close(); + }); + return http.ByteStream(controller.stream); } } @@ -236,6 +285,8 @@ class FakeApiConnection extends ApiConnection { /// /// In each case, the next request will complete a duration of `delay` /// after being started. + /// On success, the response body arrives a further `bodyDelay` + /// after the response's headers. void prepare({ Object? httpException, ZulipApiException? apiException, @@ -243,6 +294,7 @@ class FakeApiConnection extends ApiConnection { Map? json, String? body, Duration delay = Duration.zero, + Duration bodyDelay = Duration.zero, }) { assert(isOpen); @@ -279,7 +331,7 @@ class FakeApiConnection extends ApiConnection { client.prepare( exception: httpException, httpStatus: httpStatus, json: json, body: body, - delay: delay, + delay: delay, bodyDelay: bodyDelay, ); } diff --git a/test/example_data.dart b/test/example_data.dart index 820f00497..f82e50103 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -1451,6 +1451,7 @@ InitialSnapshot initialSnapshot({ int? maxFileUploadSizeMib, List? serverThumbnailFormats, Uri? serverEmojiDataUrl, + int? eventQueueLongpollTimeoutSeconds, int? realmModerationRequestChannelId = -1, String? realmEmptyTopicDisplayName, List? realmUsers, @@ -1519,6 +1520,7 @@ InitialSnapshot initialSnapshot({ serverThumbnailFormats: serverThumbnailFormats ?? [], serverEmojiDataUrl: serverEmojiDataUrl ?? realmUrl.replace(path: '/static/emoji.json'), + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds ?? 90, realmModerationRequestChannelId: realmModerationRequestChannelId, realmEmptyTopicDisplayName: realmEmptyTopicDisplayName ?? defaultRealmEmptyTopicDisplayName, realmUsers: realmUsers ?? [selfUser], diff --git a/test/model/store_test.dart b/test/model/store_test.dart index d4852153c..973fad59b 100644 --- a/test/model/store_test.dart +++ b/test/model/store_test.dart @@ -686,6 +686,44 @@ void main() { users.map((expected) => (it) => it.fullName.equals(expected.fullName))); })); + group('registerQueue error reporting', () { + String? lastReportedError; + + Future prepare() async { + lastReportedError = null; + reportErrorToUserBriefly = (message, {details}) async { + if (message == null) return; + lastReportedError = message; + }; + addTearDown(() => + reportErrorToUserBriefly = defaultReportErrorToUserBriefly); + await prepareStore(); + globalStore.useCachedApiConnections = true; + } + + /// Load, with the first registerQueue attempt failing with [exception] + /// and the retry succeeding. + Future loadWithFirstAttemptFailing(Object exception) async { + connection.prepare(httpException: exception); + connection.prepare(json: eg.initialSnapshot().toJson()); + final updateMachine = await UpdateMachine.load( + globalStore, eg.selfAccount.id); + updateMachine.debugPauseLoop(); + } + + test('no report on failed connection', () => awaitFakeAsync((async) async { + await prepare(); + await loadWithFirstAttemptFailing(const SocketException('failed')); + check(lastReportedError).isNull(); + })); + + test('report other network error', () => awaitFakeAsync((async) async { + await prepare(); + await loadWithFirstAttemptFailing(Exception('failed')); + check(lastReportedError).isNotNull(); + })); + }); + // TODO test UpdateMachine.load starts polling loop }); @@ -764,10 +802,14 @@ void main() { connection = store.connection as FakeApiConnection; } - Future preparePoll({int? lastEventId}) async { + Future preparePoll({ + int? lastEventId, + int? eventQueueLongpollTimeoutSeconds, + }) async { globalStore = eg.globalStore(); await globalStore.add(eg.selfAccount, eg.initialSnapshot( - lastEventId: lastEventId)); + lastEventId: lastEventId, + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds)); await globalStore.perAccount(eg.selfAccount.id); updateFromGlobalStore(); updateMachine.debugPauseLoop(); @@ -866,24 +908,37 @@ void main() { }); } - void checkRetry(void Function() prepareError) { + /// Check the poll loop quietly retries when [prepareError] + /// causes the request to fail. + /// + /// [elapse] is how long the request takes to fail: + /// zero for an immediate error, + /// or the poll timeout for a request that times out. + void checkRetry(void Function() prepareError, { + Duration elapse = Duration.zero, + int? eventQueueLongpollTimeoutSeconds, + }) { awaitFakeAsync((async) async { - await preparePoll(lastEventId: 1); + await preparePoll(lastEventId: 1, + eventQueueLongpollTimeoutSeconds: eventQueueLongpollTimeoutSeconds); check(async.pendingTimers).length.equals(0); // Make the request, inducing an error in it. prepareError(); updateMachine.debugAdvanceLoop(); - async.elapse(Duration.zero); + async.elapse(elapse); checkLastRequest(lastEventId: 1, expectDontBlock: false); check(store).isRecoveringEventStream.isTrue(); // Polling doesn't resume immediately; there's a timer. - check(async.pendingTimers).length.equals(1); + // (On a timed-out request, the fake's timer for the response + // that never arrived is also still pending.) + final pendingTimers = elapse == Duration.zero ? 1 : 2; + check(async.pendingTimers).length.equals(pendingTimers); updateMachine.debugAdvanceLoop(); async.flushMicrotasks(); check(connection.lastRequest).isNull(); - check(async.pendingTimers).length.equals(1); + check(async.pendingTimers).length.equals(pendingTimers); // Polling continues after a timer. connection.prepare(json: GetEventsResult(events: [ @@ -902,7 +957,9 @@ void main() { updateMachine.debugPrepareLoopError(eg.nullCheckError()); } - void prepareNetworkExceptionSocketException() { + // [ApiConnection] classifies a [SocketException] + // as [NetworkExceptionKind.connectionFailed]. + void prepareNetworkExceptionConnectionFailed() { connection.prepare(httpException: const SocketException('failed')); } @@ -969,9 +1026,18 @@ void main() { checkReload(prepareUnexpectedLoopError); }); - test('retries on NetworkException from SocketException', () { + test('retries on NetworkException from failed connection', () { // We skip reporting errors on these; check we retry them all the same. - checkRetry(prepareNetworkExceptionSocketException); + checkRetry(prepareNetworkExceptionConnectionFailed); + }); + + test('retries when request outlives the server-recommended timeout', () { + // In the wild, this is a connection that died without erroring; see #514. + checkRetry( + eventQueueLongpollTimeoutSeconds: 90, + elapse: const Duration(seconds: 90), + () => connection.prepare(delay: const Duration(seconds: 300), + json: GetEventsResult(events: [], queueId: null).toJson())); }); test('retries on generic NetworkException', () { @@ -1111,8 +1177,8 @@ void main() { checkReported(prepareUnexpectedLoopError); }); - test('ignore NetworkException from SocketException', () { - checkNotReported(prepareNetworkExceptionSocketException); + test('ignore NetworkException from failed connection', () { + checkNotReported(prepareNetworkExceptionConnectionFailed); }); test('eventually report generic NetworkException', () {