Spike: Add optional retry middleware #51
Conversation
pkg/proxy/middleware.go
Outdated
| for i := 0; ; i++ { | ||
| capturedResp = &responseBuffer{ | ||
| header: make(http.Header), | ||
| body: bytes.NewBuffer([]byte{}), |
There was a problem hiding this comment.
As we spoke about, I think it would be good to know whether we are adding to in-memory buffering or not by doing this. I think it is still worth having this feature even if we are making memory requirements worse. This makes me think that we might actually want to make retrying configurable:
- enabled/disabled
- list of status codes to retry? maybe this configurable item would be excessive?
pkg/proxy/handler.go
Outdated
| if err != nil { | ||
| return "", r, nil | ||
| var body []byte | ||
| if mb, ok := r.Body.(*lazyBodyCapturer); ok && mb.capturedBody != nil { |
There was a problem hiding this comment.
What is this change doing?
There was a problem hiding this comment.
With this change, I am re-using the buffer of the retry middleware so that the body must not be re-read (and buffered) again. It is an optimization
pkg/proxy/middleware.go
Outdated
| return d | ||
| } | ||
|
|
||
| func (r *responseWriterDelegator) discardedResponse() bool { |
There was a problem hiding this comment.
Seems like this method could go away in favor of just checking r.discardErrResp?
There was a problem hiding this comment.
I need this to satisfy the response type in newResponseWriterDelegator() xResponseWriter. The source writer either implements ReaderFrom or not. So does the returned implementation.
| headerBuf: make(http.Header), | ||
| discardErrResp: discardErrResp, // abort early on timeout, context cancel | ||
| } | ||
| if _, ok := writer.(io.ReaderFrom); ok { |
There was a problem hiding this comment.
In Lingo when would the writer be an io.ReaderFrom and when would it not be?
There was a problem hiding this comment.
The response type in the Go stdlib implements the interface. So should do all decorators to make use of internal or future optimizations. it is used in io.Copy for example. I saw a presentation once but I don't have the link anymore. I also double checked with the prometheus decorator to ensure it is still a good practice
Same for WriteTo on the lazyBodyCapturerWriteTo
| // If the maximum number of retries is 0, it returns the next handler without adding any retries. | ||
| // If the list of retryable status codes is empty, it uses a default set of status codes (http.StatusBadGateway, http.StatusServiceUnavailable, http.StatusGatewayTimeout). | ||
| // The function creates a RetryMiddleware struct with the given parameters and returns it as an http.Handler. | ||
| func NewRetryMiddleware(maxRetries int, other http.Handler, optRetryStatusCodes ...int) http.Handler { |
There was a problem hiding this comment.
We need to limit the max bytes read. See https://cwe.mitre.org/data/definitions/400.html example 6
There was a problem hiding this comment.
I can see this being useful.
alpe
changed the title
|
Closing this now in favour of #61 |
Allow for retrying failed requests to backends. Default to 1 retry per lingo-request. Fixes #48 Builds on work done by @alpe in #64 and #51. Co-authored-by: Alex Peters <[email protected]>
Resolves #48
the implementation is based on 2 elements:
not considered is a priority queue for retries