firebase-admin-go icon indicating copy to clipboard operation
firebase-admin-go copied to clipboard

Should the `internal.HTTPClient` retry when Google's servers return 502 default for the Messaging Endpoint?

Open huyi1985 opened this issue 1 year ago • 2 comments
trafficstars

When Google's servers reponse 502 for the Messaging Endpoint(https://fcm.googleapis.com/v1) requests, the response body has been read in advance and the RetryConfig can catch underlying network errors, as the following comment says,

// https://github.com/firebase/firebase-admin-go/blob/master/internal/http_client.go#L194
// ...
	// If a RetryConfig is available, always consult it to determine if the request should be retried
	// or not. Even if there was a network error, we may not want to retry the request based on the
	// RetryConfig that is in effect.
	if c.RetryConfig != nil {
		delay, retry := c.RetryConfig.retryDelay(retries, resp, result.Err)
		result.RetryAfter = delay
		result.Retry = retry
	}

But for the choice of most users, the default retry config WithDefaultRetryConfig only retries when http.StatusServiceUnavailable 503 occurred.

// https://github.com/firebase/firebase-admin-go/blob/master/internal/http_client.go#L82
func WithDefaultRetryConfig(hc *http.Client) *HTTPClient {
	twoMinutes := time.Duration(2) * time.Minute
	return &HTTPClient{
		Client: hc,
		RetryConfig: &RetryConfig{
			MaxRetries: 4,
			CheckForRetry: retryNetworkAndHTTPErrors(
				http.StatusServiceUnavailable,
			),
// ...

And it is true that Google often returns 502 for the Messaging Endpoint requests as shown below,

<!DOCTYPE html>
<html lang=en>
  <meta charset=utf-8>
  <meta name=viewport content="initial-scale=1, minimum-scale=1, width=device-width">
  <title>Error 502 (Server Error)!!1</title>
  <style>
    *{margin:0;padding:0}html,code{font:15px/22px arial,sans-serif}html{background:#fff;color:#222;padding:15px}body{margin:7% auto 0;max-width:390px;min-height:180px;padding:30px 0 15px}* > body{background:url(//www.google.com/images/errors/robot.png) 100% 5px no-repeat;padding-right:205px}p{margin:11px 0 22px;overflow:hidden}ins{color:#777;text-decoration:none}a img{border:0}@media screen and (max-width:772px){body{background:none;margin-top:0;max-width:none;padding-right:0}}#logo{background:url(//www.google.com/images/branding/googlelogo/1x/googlelogo_color_150x54dp.png) no-repeat;margin-left:-5px}@media only screen and (min-resolution:192dpi){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat 0% 0%/100% 100%;-moz-border-image:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) 0}}@media only screen and (-webkit-min-device-pixel-ratio:2){#logo{background:url(//www.google.com/images/branding/googlelogo/2x/googlelogo_color_150x54dp.png) no-repeat;-webkit-background-size:100% 100%}}#logo{display:inline-block;height:54px;width:150px}
  </style>
  <a href=//www.google.com/><span id=logo aria-label=Google></span></a>
  <p><b>502.</b> <ins>That’s an error.</ins>
  <p>The server encountered a temporary error and could not complete your request.<p>Please try again in 30 seconds.  <ins>That’s all we know.</ins>

huyi1985 avatar Aug 26 '24 09:08 huyi1985

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

google-oss-bot avatar Aug 26 '24 09:08 google-oss-bot

Hey @chong-shao do you think we should retry 502 errors?

lahirumaramba avatar Aug 28 '24 15:08 lahirumaramba