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

Added support for HTTP 429 Retry-After as per Issue #99 #100

Merged
merged 16 commits into from
Jun 22, 2020

Conversation

mariotoffia
Copy link
Contributor

This was added in DefaultPolicy and DefaultBackoff. The latter will
examine the Retry-After response header (if existant) and do a backoff
for the specified amount of time. Otherwise it will default to default
backoff behaviour.

This addresses Issue #99.

Mario Toffia added 2 commits June 11, 2020 08:10
This was added in DefaultPolicy and DefaultBackoff. The latter will
examine the Retry-After response header (if existant) and do a backoff
for the specified amount of time. Otherwise it will default to default
backoff behaviour.
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 11, 2020

CLA assistant check
All committers have signed the CLA.

@theckman
Copy link
Contributor

theckman commented Jun 13, 2020

There's PR #70 which adds a new retry policy function, based on the one in master. If that gets merged before this one, we'll need to update this PR to include adding it in the new function (in 70).

@jefferai
Copy link
Member

@mariotoffia #70 was merged. Additionally the CLA isn't signed. Please update/fix and we can look again!

@mariotoffia
Copy link
Contributor Author

Thanks! :)

I've merged and committed - still there's two test errors The report I could download (Artifacts) do not describe the output of the error, just that it has errors on following methods:

<testsuites>
<testsuite tests="3" failures="0" time="0.000000" name="github.com/hashicorp/go-retryablehttp">
<properties>
<property name="go.version" value="go1.14.2 linux/amd64"/>
</properties>
<testcase classname="" name="TestMain" time="0.000000">
<failure message="Failed" type="">FAIL github.com/hashicorp/go-retryablehttp 0.007s </failure>
</testcase>
<testcase classname="github.com/hashicorp/go-retryablehttp" name="TestRequest" time="0.000000"/>
<testcase classname="github.com/hashicorp/go-retryablehttp" name="TestFromRequest" time="0.000000"/>
</testsuite>
</testsuites>

I don't know why those two functions get's errors since I've not altered those. If I e.g. run TestFromRequest locally it works just fine:

go test -v -run TestFromRequest
=== RUN   TestFromRequest
--- PASS: TestFromRequest (0.00s)
PASS
ok      github.com/hashicorp/go-retryablehttp   0.004s

Cheers,
Mario

@mateusrangel
Copy link

mateusrangel commented Jun 17, 2020

Seems like he didn't signed the CLA and one test is failing.

Any news?

@mateusrangel
Copy link

So @mariotoffia, i am checking your code and already detected the point of panic.

@mateusrangel
Copy link

locally you are testing only TestFromRequest, i did a pure go test before and after your changes and got a

[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x6db207]

goroutine 9 [running]:
github.com/hashicorp/go-retryablehttp.DefaultBackoff(0x989680, 0x2faf080, 0x0, 0x0, 0x3)

When i comment your code at:

if resp.StatusCode == http.StatusTooManyRequests {
	if s, ok := resp.Header["Retry-After"]; ok {
		if sleep, err := strconv.ParseInt(s[0], 10, 32); err == nil {
			return time.Duration(int64(time.Second) * sleep)
		}
	}
}

every thing runs fine, seems like we have a problem in the header check or in the parsing

@mariotoffia
Copy link
Contributor Author

@mateusrangel That's strange - when I do a

martoffi@dell:~/progs/github/go-retryablehttp$ go test -v -run TestClient_DefaultBackoff429TooManyRequest

I do get

=== RUN   TestClient_DefaultBackoff429TooManyRequest
2020/06/17 21:10:10 [DEBUG] GET http://127.0.0.1:40959
--- PASS: TestClient_DefaultBackoff429TooManyRequest (0.00s)
PASS
ok      github.com/hashicorp/go-retryablehttp   0.005s

When I run it locally. However, I was a bit curious how to get the test output from your CI tool - how do I do that?

Cheers,
Mario

@mariotoffia
Copy link
Contributor Author

@mateusrangel Ok, I've got it - the

resp *http.Response

is nil. I'm committing a fix now.

@mateusrangel
Copy link

@mariotoffia Happy to help the debugging 😄

@mariotoffia
Copy link
Contributor Author

mariotoffia commented Jun 18, 2020

@jefferai is it good to go?
(the core of it is here)

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
client_test.go Outdated Show resolved Hide resolved
mariotoffia and others added 4 commits June 18, 2020 17:33
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
mariotoffia and others added 3 commits June 18, 2020 17:35
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>
@mariotoffia
Copy link
Contributor Author

@jefferai committed your review points - I'd guess this PR needs to be squashed since it is insane amount of commits :)

client.go Outdated Show resolved Hide resolved
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jefferai jefferai merged commit 1831df7 into hashicorp:master Jun 22, 2020
jefferai pushed a commit that referenced this pull request Nov 4, 2020
* Realign behavior of ErrorPropagatedRetryPolicy with DefaultRetryPolicy

ErrorPropagatedRetryPolicy should have been modified in #100,
but failed to... probably because it has been merged approximately at the same time
as #70 (the PR adding ErrorPropagatedRetryPolicy).

* Remove code duplication between DefaultRetryPolicy and ErrorPropagatedRetryPolicy
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.

None yet

5 participants