api: Time out long-poll requests, as the server recommends

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<String, dynamic>?;
+    } 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<T> get<T>(String routeName, T Function(Map<String, dynamic>) fromJson,
-      String path, Map<String, dynamic>? params) async {
+      String path, Map<String, dynamic>? 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<T> _withTimeout<T>(Duration timeout,
+      Future<T> Function(Future<void> abortTrigger) body) async {
+    final abortCompleter = Completer<void>();
+    final abortTimer = Timer(timeout, abortCompleter.complete);
+    try {
+      return await body(abortCompleter.future);
+    } finally {
+      abortTimer.cancel();
+    }
   }
 
   Future<T> post<T>(String routeName, T Function(Map<String, dynamic>) 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<String, dynamic>? json) {
   assert(httpStatus != 200 || json == null);
   if (400 <= httpStatus && httpStatus <= 499) {

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() {

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,

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<GetEventsResult> 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)

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

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():

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<NetworkException>((it) => it
+          ..routeName.equals(kExampleRouteName)
+          ..kind.equals(.connectionFailed)
+          ..cause.isA<http.RequestAbortedException>());
+    });
+  }));
+
+  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<NetworkException>((it) => it
+          ..routeName.equals(kExampleRouteName)
+          ..kind.equals(.connectionFailed)
+          ..cause.isA<http.RequestAbortedException>());
+    });
+  }));
+
+  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<void> 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<T> tryRequest<T extends Object?>({
   Object? exception,
   int? httpStatus,

test/api/exception_checks.dart

@@ -14,6 +14,7 @@ extension ZulipApiExceptionChecks on Subject<ZulipApiException> {
 }
 
 extension NetworkExceptionChecks on Subject<NetworkException> {
+  Subject<NetworkExceptionKind> get kind => has((e) => e.kind, 'kind');
   Subject<Object> get cause => has((e) => e.cause, 'cause');
 }