Timeout should be handled by the http client not the library

[RobertBrunhage]

Having timeouts with requests doesn't propagate through links and in general has caused weird behaviors such as causing timeout on app start (even though manually setting timeout to 60 sec). As it's not getting passed through the links as an exception it's no reasonable way to have it trigger a RetryLink either.

graphql-flutter/packages/graphql/lib/src/core/query_manager.dart

Line 260 in 59bb1d5

await link.request(request).timeout(Duration(seconds: 5)).first;

removing the timeout here and controlling the timeout on the http client gave the correct response with it propagating through the links for retries and error reporting.

To Reproduce (MUST BE PROVIDED)
Make a request that is longer than the timeout (you can set the timeout to 1 sec) and check if ErrorLink can print it.

Expected behavior
timeout should be controlled through the http client and timeouts should pass through the links.

device / execution context
iOS/Android

[RobertBrunhage]

Also, having 5 seconds as a default timeout duration is a very weird choice. Any request on a slow network will cause timeouts that is not logged through ErrorLink etc.

[rwrz]

I agree! I was surprised by this today.
But my timeout implementation was done as an HttpLink. If anyone is interested (as is):

import 'dart:async';
import 'dart:math';

import 'package:common_dart/core/offline/connectivity_cubit.dart';
import 'package:graphql/client.dart';
import 'package:graphql_flutter/graphql_flutter.dart';
import 'package:http/http.dart' as http;
import 'package:logging/logging.dart';
import 'package:rxdart/rxdart.dart';

class HttpTimeoutRetry extends HttpLink {
  HttpTimeoutRetry(
    super.uri,
    this._connectivityCubit, {
    this.requestTimeout = const Duration(seconds: 15),
    super.defaultHeaders = const <String, String>{},
    super.useGETForQueries = false,
    http.Client? httpClient,
    super.serializer = const RequestSerializer(),
    super.parser = const ResponseParser(),
    super.followRedirects = false,
  }) : super(httpClient: httpClient);

  final Logger _loggerService = Logger('HttpTimeoutReTry');

  final Duration requestTimeout;
  final int maxRetries = 3;
  final ConnectivityCubit _connectivityCubit;

  @override
  Stream<Response> request(Request request, [NextLink? forward]) async* {
    Duration currentTimeout = Duration(microseconds: requestTimeout.inMicroseconds);
    int retries = 0;

    final Stream<Response> timeoutStream = RetryWhenStream<Response>(() {
      // only retry queries, to avoid issues with the current mutation/command logic
      if (request.isQuery) {
        return super.request(request, forward).timeout(currentTimeout);
      } else {
        return super.request(request, forward);
      }
    }, (Object error, StackTrace stack) {
      if (error is! TimeoutException) {
        return Stream<void>.error(error, stack);
      }

      if (retries < maxRetries) {
        retries++;
        // after the backend fixes, the timeout should still exists, but only for extreme scenarios
        // so, we put only 3 re-tries, adding 15 seconds on each.
        //if (retries > 3) {
        currentTimeout = Duration(seconds: currentTimeout.inSeconds + 15);
        // } else {
        //   currentTimeout = Duration(seconds: currentTimeout.inSeconds + 2);
        // }
      } else {
        return Stream<void>.error(error, stack);
      }

      // we will retry
      _loggerService.fine(
          'RETRYING, waiting: $requestTimeout, current $retries of $maxRetries - ${request.operation.operationName} - online?: ${_connectivityCubit.state}');

      // check for connectivity
      if (_connectivityCubit.state is OfflineState) {
        _loggerService.fine('waiting connectivity');
        // if offline, only retry after it is online again
        return _connectivityCubit.stream.where((ConnectivityState e) => e is OnlineState);
      }

      // if online:
      // avoid herd issues (if a server goes down, some clients might wait different moments to retry) -random, from 0 to 500ms
      return Rx.timer<void>(null, Duration(milliseconds: Random().nextInt(500)));
    });

    yield* timeoutStream;
  }
}

[hanskokx]

I just spent all day trying to figure out why DevTools was showing me a query response but the client had an exception with no data. Thank you a million times for helping point me in the right direction!

[leocape]

+1 this should be customisable per request ideally (or at least at client creation)

edit: for graphql package (not graphql-flutter) this fix has been merged to main graphql-v5.2.0-beta.9, thanks!

  return GraphQLClient(
      link: httpLink,
      cache: GraphQLCache(),
      **queryRequestTimeout: const Duration(seconds: 30),**
    );

[muhammadkamel]

Please, publish the "graphql-v5.2.0-beta.9" version to the pub.dev, to allow us to change the timeout duration, ASAP.

[leocape]

It is there already

https://pub.dev/packages/graphql/versions/5.2.0-beta.9

[muhammadkamel]

I see.

I am taking about the "graphql flutter" package:
https://pub.dev/packages/graphql_flutter

[leocape]

Ah whoops sorry was cross posting on the graphql GH :)

[andrewmumm-wk]

The addition of this timeout has caused most of my unit tests to now fail regardless of the timeout set.

For example:

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following assertion was thrown running a test:
A Timer is still pending even after the widget tree was disposed.
'package:flutter_test/src/binding.dart':
Failed assertion: line 1542 pos 12: '!timersPending'

This is regardless of the timeout being set directly or passed in through the client.

The following returns all tests to working order.

- response = await link.request(request).timeout(this.requestTimeout).first;
+ response = await link.request(request).first;

[vincenzopalazzo]

There is someone that can open a PR to fix this issue?

[woutervanwijk]

5 seconds as a default is indeed way to low. I really had to dig deep to get here

you can fix it for now by forcing graphql to beta.8 in pubspec.yaml:

graphql: 5.2.0-beta.8

[mohammad-quanit]

i am using graphql_flutter: ^5.2.0-beta.6 still getting that 5 second issue. Unable to process graphql queries. Any solution?

[vincenzopalazzo]

@woutervanwijk @mohammad-quanit what about #1457 (comment) ?

It is working with the latest one?

[mohammad-quanit]

@vincenzopalazzo I am using graphql_flutter package and installed the latest version but still getting this issue.

QueryResult(source: QueryResultSource.network, data: null, context: Context({}), exception: OperationException(linkException: UnknownException(TimeoutException after 0:00:05.000000: No stream event, stack:<…>

[vincenzopalazzo]

@mohammad-quanit probably you have to change the default timeout?

[mohammad-quanit]

@vincenzopalazzo can you guide me how? where to change timeout?

[vincenzopalazzo]

Try to see if this solve your problem https://github.com/zino-hofmann/graphql-flutter/pull/1447/files

if now a PR will be welcome that specify your specific use case :) I am trying to be responsive in PR review and library release.

Hoping to drop the v5.2 at the end of the year and drop the beta