Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only try read resp if there was no error #794

Closed
wants to merge 1 commit into from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Sep 13, 2020

Fixes #790

** NOT YET TESTED**

Warn Checklist

  • This changes the interface and requires a Major/Minor version change.
  • I have 🎩'd these changes by using the commands I changed by hand.
  • I have added a dependancy to the project.
  • I have considered any potential impact on node-themekit

@@ -127,7 +127,7 @@ func (client *HTTPClient) doWithRetry(req *http.Request, body interface{}) (*htt
return resp, fmt.Errorf("request timed out after %v retries, there may be an issue with your connection", client.maxRetry)
}
time.Sleep(time.Duration(attempt) * time.Second)
} else if resp.StatusCode == http.StatusTooManyRequests {
} else if err == nil && resp.StatusCode == http.StatusTooManyRequests {
after, _ := strconv.ParseFloat(resp.Header.Get("Retry-After"), 10)
client.limit.ResetAfter(time.Duration(after))
} else if err != nil && strings.Contains(err.Error(), "no such host") {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tanema it looks like if there's an error other than "no such host", then we ignore it? Would it be helpful to return an error here to aid future troubleshooting?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not ignored, it will just be retried.

@andyw8 andyw8 force-pushed the andyw8/fix-handling-of-nil-resp branch from 8817950 to 0ccb7cf Compare September 22, 2020 19:17
@tanema
Copy link
Contributor

tanema commented Sep 28, 2020

I have taken care of this in #809 because the problem was a lot larger than this.

@tanema tanema closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when checking response status code
2 participants