-
Notifications
You must be signed in to change notification settings - Fork 94
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
fix(forwarding): Change HTTP response for upstream timeouts from 502 to 504 #859
Conversation
@@ -75,6 +75,7 @@ impl ResponseError for ForwardedUpstreamRequestError { | |||
HttpError::ActixPayload(e) => e.error_response(), | |||
HttpError::ActixJson(e) => e.error_response(), | |||
}, | |||
UpstreamRequestError::SendFailed(_) => HttpResponse::GatewayTimeout().finish(), |
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.
This should be a 500
instead of a 502
, since it indicates a failure to build the request, correct?
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.
this error actually needs more destructuring
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.
Can we instead map all relevant errors to UpstreamRequestError
kinds? It is rather unfortunate having to destructure errors like this since it is too easy to miss cases, as shown by this PR.
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 is a change in behavior, it would be great to add a changelog entry before merging.
@untitaker can you update this? |
e34755d
to
e23f36f
Compare
Fix RELAY-93