Move HTTP client creation inside the API client retry callback block.

[isoos]

No description provided.

[jonasfj]

I have no objections, but I also don't see what possible good this could do.

(A) I'm assuming this is only used in testing.
(B) An http.Client should not need to be recreated between retries, it's a connection pool, if there is a broken connection it'll be closed.

So I don't really understand what this would do (what am I missing?)

[isoos]

I'm going to introduce a withRetryHttpClient wrapper, that would be used in the non-testing code too, following the same principles as here: may create a client inside of it, retries on various exception (not like the current RetryClient). I thought that a natural first step would be to make this method into the shape of the new wrapper so the transition step is easier to follow.

[isoos]

An http.Client should not need to be recreated between retries, it's a connection pool, if there is a broken connection it'll be closed.

Though, this is a bug here: we create a Client, we try to use inside a retry block, and on any exception, we close it. However, the next retry round is now going to be run on a closed client.

[isoos]

Hm, the planned refactor of the withRetryHttpClient method is tought. This is a crude version of it:

Future<R> _withRetryHttpClient<R>(
  /// The callback function that may be retried on transient errors.
  Future<R> Function(http.Client client) fn, {
  FutureOr<http.Client> Function()? clientFn,
  http.Client? client,
}) async {
  return await retry(
    () async {
      final skipClosing = client != null;
      http.Client? httpClient;
      try {
        httpClient = client ?? await clientFn!();
        return await fn(httpClient);
      } finally {
        if (!skipClosing) {
          httpClient?.close();
        }
      }
    },
    maxAttempts: 3,
    retryIf: _retryIf,
  );
}

However, if we use client, then it is not recreated on retries, which may be needed to skip a stale connection, and if we use clientFn, then it is recreated all the time, which may be okay for one-off tasks, but is not that beneficial for e.g. search client connections. wdyt? Should we rather have a state tracking wrapper inside a custom class or a subclass of http.Client?