-
Notifications
You must be signed in to change notification settings - Fork 239
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
Re-sign request on retry #216
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c872c98
DXE-3480 Re-sign request on retry
mgwoj b2e8d9b
DXE-3480 Configurable retry
mgwoj 25093ac
DXE-3480 Unit test
mgwoj 8eb50ac
DXE-3480 Re-sign request on retry
mgwoj c5939ed
Update go.mod
mgwoj edadfe1
DXE-3480 Simplify code for PrepareRetry
mgwoj File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -393,6 +393,9 @@ type Backoff func(min, max time.Duration, attemptNum int, resp *http.Response) t | |||||||||||||||||||||
// attempted. If overriding this, be sure to close the body if needed. | ||||||||||||||||||||||
type ErrorHandler func(resp *http.Response, err error, numTries int) (*http.Response, error) | ||||||||||||||||||||||
|
||||||||||||||||||||||
// PrepareRetry is called before retry operation. It can be used for example to re-sign the request | ||||||||||||||||||||||
type PrepareRetry func(req *http.Request) error | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Client is used to make HTTP requests. It adds additional functionality | ||||||||||||||||||||||
// like automatic retries to tolerate minor outages. | ||||||||||||||||||||||
type Client struct { | ||||||||||||||||||||||
|
@@ -421,6 +424,9 @@ type Client struct { | |||||||||||||||||||||
// ErrorHandler specifies the custom error handler to use, if any | ||||||||||||||||||||||
ErrorHandler ErrorHandler | ||||||||||||||||||||||
|
||||||||||||||||||||||
// PrepareRetry can prepare the request for retry operation, for example re-sign it | ||||||||||||||||||||||
PrepareRetry PrepareRetry | ||||||||||||||||||||||
|
||||||||||||||||||||||
loggerInit sync.Once | ||||||||||||||||||||||
clientInit sync.Once | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
@@ -435,6 +441,7 @@ func NewClient() *Client { | |||||||||||||||||||||
RetryMax: defaultRetryMax, | ||||||||||||||||||||||
CheckRetry: DefaultRetryPolicy, | ||||||||||||||||||||||
Backoff: DefaultBackoff, | ||||||||||||||||||||||
PrepareRetry: DefaultPrepareRetry, | ||||||||||||||||||||||
} | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -551,6 +558,12 @@ func DefaultBackoff(min, max time.Duration, attemptNum int, resp *http.Response) | |||||||||||||||||||||
return sleep | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// DefaultPrepareRetry is performing noop during prepare retry | ||||||||||||||||||||||
func DefaultPrepareRetry(_ *http.Request) error { | ||||||||||||||||||||||
// noop | ||||||||||||||||||||||
return nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not needed if we accommodate a nil value for |
||||||||||||||||||||||
// LinearJitterBackoff provides a callback for Client.Backoff which will | ||||||||||||||||||||||
// perform linear backoff based on the attempt number and with jitter to | ||||||||||||||||||||||
// prevent a thundering herd. | ||||||||||||||||||||||
|
@@ -618,10 +631,10 @@ func (c *Client) Do(req *Request) (*http.Response, error) { | |||||||||||||||||||||
var resp *http.Response | ||||||||||||||||||||||
var attempt int | ||||||||||||||||||||||
var shouldRetry bool | ||||||||||||||||||||||
var doErr, respErr, checkErr error | ||||||||||||||||||||||
var doErr, respErr, checkErr, prepareErr error | ||||||||||||||||||||||
|
||||||||||||||||||||||
for i := 0; ; i++ { | ||||||||||||||||||||||
doErr, respErr = nil, nil | ||||||||||||||||||||||
doErr, respErr, prepareErr = nil, nil, nil | ||||||||||||||||||||||
attempt++ | ||||||||||||||||||||||
|
||||||||||||||||||||||
// Always rewind the request body when non-nil. | ||||||||||||||||||||||
|
@@ -728,17 +741,24 @@ func (c *Client) Do(req *Request) (*http.Response, error) { | |||||||||||||||||||||
// without racing against the closeBody call in persistConn.writeLoop. | ||||||||||||||||||||||
httpreq := *req.Request | ||||||||||||||||||||||
req.Request = &httpreq | ||||||||||||||||||||||
|
||||||||||||||||||||||
if err := c.PrepareRetry(req.Request); err != nil { | ||||||||||||||||||||||
prepareErr = err | ||||||||||||||||||||||
break | ||||||||||||||||||||||
} | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the above, can we conditionally attempt this if the function is non-nil?
Suggested change
|
||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
// this is the closest we have to success criteria | ||||||||||||||||||||||
if doErr == nil && respErr == nil && checkErr == nil && !shouldRetry { | ||||||||||||||||||||||
if doErr == nil && respErr == nil && checkErr == nil && prepareErr == nil && !shouldRetry { | ||||||||||||||||||||||
return resp, nil | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
defer c.HTTPClient.CloseIdleConnections() | ||||||||||||||||||||||
|
||||||||||||||||||||||
var err error | ||||||||||||||||||||||
if checkErr != nil { | ||||||||||||||||||||||
if prepareErr != nil { | ||||||||||||||||||||||
err = prepareErr | ||||||||||||||||||||||
} else if checkErr != nil { | ||||||||||||||||||||||
err = checkErr | ||||||||||||||||||||||
} else if respErr != nil { | ||||||||||||||||||||||
err = respErr | ||||||||||||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this default function does nothing, can we leave the default as
nil
here?