Refactor HTTP retries (#3325)

[nehalvpatel]

UPDATE FROM 10/26/2022 - Plan:

• http.dart exposes:
  • retryHttpLogic / retryForHttp
    • Accepts a function argument
    • Retries with exponential backoff. Default max = 7
    • Does retry when:
      • HTTP 408, 429
      • HTTP 5xx errors
      • HttpException, SocketException, TlsException, WebSocketException, PubHttpException if PubHttpException.isIntermittent
        • PackageIntegrityException will subclass PubHttpException and be used for call-site specific errors. For example: crc32c validation and sha256 validation
    • If retries fail, throws ApplicationException (investigation of existing behavior needed)
      • 404 by solver (PubHttpException)
      • SocketException?
    • Logs exceptions at a high level (DEBUG)?
    • Logs retry log at a high level (INFO/DEBUG)?
  • extension on http.BaseResponse called throwIfNotOk()
  • globalHttpClient
  • attachRequestMetadata(http.Request request)
    • attaches the x-pub-os / command, etc.. headers from zone variables
    • Stuff that _PubHttpClient currently does
    • Used when making requests to list-versions and download archive
• in oauth2.dart, wrap http.Client in RetryClient and pass to package:oauth2

---

The goal is to simplify and condense the retry logic. A future PR will address the other items in #3325.

[jonasfj]

This looks really good, I only have nits to suggest.

[jonasfj]

If you have to cast, maybe it's better to do extension Throwing on http.Response or, make PubHttpResponseException accept an BaseResponse

[nehalvpatel]

Extending http.Response breaks the extension for the _download method, which uses http.StreamedResponse. Vice-versa for if we extend http.StreamedResponse, since the versions method uses http.Response.

---

The change to make PubHttpResponseException accept http.BaseResponse works, but causes build errors in lish.dart:

---

If we change handleJsonError to accept http.BaseResponse, we get some more errors:

---

And then finally, if we change parseJsonResponse to accept http.BaseResponse:

---

So, the cast was the only way I could come up with to make it all work. 😅

[jonasfj]

Fair point, we can clean it up later.

[jonasfj]

One option could be to create PubHttpResponse(BaseResponse, List<int>? body); so that it has an optional body.

But maybe this is better left for a follow up PR...

[jonasfj]

I think we actually have: #3130

For refactoring handleJsonError, I think if we're going to make better output -- that's also a great opportunity to re-think the API interface and use something like BaseResponse instead.

---

Actually, it might be cleaner to cast from BaseResponse to Response in handleJsonError, because -- if it's not a Response then it's same as not being able to read the body. So we could do something like:

void handleJsonError(http.BaseResponse response) {
  if (response is http.Response) { // this is a cast, but also handle cases where the cast fails
    var errorMap = parseJsonResponse(response);
    ... // what we did before
  }
  invalidServerResponse(response);
}

[nehalvpatel]

The cast from BaseResponse to Response looks good!

I ended up having to implement that in this PR because the cast in throwIfNotOk was causing tests to fail.

pub/lib/src/http.dart

Lines 192 to 205 in 7afe52c

	void handleJsonError(http.BaseResponse response) {
	if (response is! http.Response) {
	// Not likely to be a common code path, but necessary.
	// See https://github.com/dart-lang/pub/pull/3590#discussion_r1012978108
	fail(log.red('Invalid server response'));
	}
	var errorMap = parseJsonResponse(response);
	if (errorMap['error'] is! Map ||
	!errorMap['error'].containsKey('message') ||
	errorMap['error']['message'] is! String) {
	invalidServerResponse(response);
	}
	fail(log.red(errorMap['error']['message']));
	}

[jonasfj]

We can't use this symbol here, I at-least I don't think we can. Since it's a private symbol, it should be distinct from the symbol defined in http.dart:

pub/lib/src/http.dart

Line 138 in 86536fd

return runZoned(callback, zoneValues: {#_dependencyType: type});

At-least that's my guess.

---

May I suggest that we make:

extension AttachHeaders on http.Request {
  void attachMetadataHeaders() {
      ...
  }
}

in http.dart.

We'll have to refactor the places that use Client.get and Client.read into creating Request objects and passing them to Client.send.
But I don't think that matters much.

[sigurdm]

Makes me wonder if we have any tests covering the meta-data sending?

It would probably require changing isPubDevUrl to something like:

  static bool isPubDevUrl(String url) {
    final origin = Uri.parse(url).origin;

    return origin == pubDevUrl ||
        origin == pubDartlangUrl ||
        (runningFromTest &&
            origin == io.Platform.environment['_PUB_TEST_DEFAULT_HOSTED_URL']);
  }

[nehalvpatel]

I've changed the headers method to an extension in http.dart. 👍

Re: the metadata test, I'd like to do that test but need some clarification on the desired behavior for the metadata URL-checking logic. I've sent you a message on Chat.

[jonasfj]

How about we do tests for metadata sending a follow up PR (it's not tests we currently have). And it's probably better to keep this PR small.

[sigurdm]

Looks really good!

We don't have to do the metadata tests in this PR - bet they surely would be nice to have

[sigurdm]

Makes me wonder if we have any tests covering the meta-data sending?

It would probably require changing isPubDevUrl to something like:

  static bool isPubDevUrl(String url) {
    final origin = Uri.parse(url).origin;

    return origin == pubDevUrl ||
        origin == pubDartlangUrl ||
        (runningFromTest &&
            origin == io.Platform.environment['_PUB_TEST_DEFAULT_HOSTED_URL']);
  }

[jonasfj]

@nehalvpatel I think if we fix the tests and retry in publishing, then this is pretty much good to go.

We can revisit the types in PubHttpResponse in a follow up PR, maybe while try to fix #3130. It's probably a lot easier to do a few smaller PRs.

[jonasfj]

I don't mind, but I have mixed feelings -- mostly because it's easy to call send() by accident instead of using fetch().

or rather it's not very obvious that fetch is intended to throw something in certain cases. But maybe, this is something we can refactor later.

[jonasfj]

We could do:

class PubHttpClient {
  Future<http.Response> fetch(http.BaseRequest request);
  Future<http.StreamingResponse> fetchStream(http.BaseRequest request);

  // For the new cases where you really need it.
  http.Client rawClient();
}

But maybe, we do this sort of thing later. Now that we're moving package:oauth2 into pub -- we could consider refactoring all of that, and just have a fetch method and fetchStream method. But it's a lot more refactoring, let's cleanup later.