Retries - ReverseProxy.ErrorHandler based approach #61
Conversation
nstogner
force-pushed
the
retries
branch
4 times, most recently
from
cf732f9 to
5f54718
Compare
nstogner
changed the title
nstogner
changed the title
pkg/proxy/request.go
Outdated
|
|
||
| func (pr *proxyRequest) httpRequest() *http.Request { | ||
| clone := pr.r.Clone(pr.r.Context()) | ||
| clone.Body = io.NopCloser(bytes.NewReader(pr.body)) |
There was a problem hiding this comment.
In the scenario with x-model header set, this behaviour is not correct. The body is empty and the original Reader overwritten.
it may also be worth to optimize for the happy path (without a retry) and lazy fill the buffer (with a TeeReader for example).
There was a problem hiding this comment.
Good catch, I'll look at the TeeReader approach. I think we could also do a nil check on pr.body before resetting close.Body here as well.
There was a problem hiding this comment.
Took the approach of checking for nil
There was a problem hiding this comment.
Also added a test case for making sure the right response body was reaching the backend.
There was a problem hiding this comment.
I think this is fixed now. I included a test case for this.
pkg/proxy/handler_test.go
Outdated
| backendCode: http.StatusInternalServerError, | ||
| backendBody: `{"err":"oh no!"}`, | ||
| expCode: http.StatusBadGateway, | ||
| expBody: `{"error":"Bad Gateway"}` + "\n", |
There was a problem hiding this comment.
When retry is enabled, the original backend error msg is never passed to the client. This can be a valid design decision but we should be sensible what error codes are "retryable". Do you have some scenarios for 500 in mind that can succeed on a retry?
There was a problem hiding this comment.
Good catch, we should probably only filter 500s
There was a problem hiding this comment.
Scratch that, I am going to just pipe through the response in the case where it got an http response by filtering out retry errors.
nstogner
changed the title
pkg/proxy/handler.go
Outdated
| // This point could be reached if a bad response code was sent by the backend | ||
| // or | ||
| // if there was an issue with the connection and no response was ever received. | ||
| if err != nil && pr.attempt < h.MaxRetries { |
There was a problem hiding this comment.
It would be good to abort early when the request context is canceled. I have debugged retries in #65 that never hit a backend
| if err != nil && pr.attempt < h.MaxRetries { | |
| if err != nil && | |
| r.Context().Err() == nil && | |
| pr.attempt < h.MaxRetries { |
There was a problem hiding this comment.
It looks like there are checks for the context being done in the reverse proxy .ServeHTTP function. Do you know what cases it would fix to also do the check here?
There was a problem hiding this comment.
🤔 the result to the end use may be the same but you get there with fewer steps. Your code would be in charge instead of relying on the stdlib proxy. I stumbled upon this when I was debugging and saw the loop iteration but no backend hit.
|
@alpe Added a test case in the handler tests for the failure you mentioned. Seeing it now. |
pkg/proxy/handler.go
Outdated
| // This point could be reached if a bad response code was sent by the backend | ||
| // or | ||
| // if there was an issue with the connection and no response was ever received. | ||
| if err != nil && pr.attempt < h.MaxRetries { |
There was a problem hiding this comment.
🤔 the result to the end use may be the same but you get there with fewer steps. Your code would be in charge instead of relying on the stdlib proxy. I stumbled upon this when I was debugging and saw the loop iteration but no backend hit.
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.