-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add redirect support for ASIO client #373
Conversation
* Add a checking task that determines whether to launch a new request with the redirection * Adheres to RFC-2068 10.3 by not allowing more than 5 redirects * Does not allow redirect loops
93bea63
to
852fbb6
Compare
@stgates would it be possible to give this a little looksee? |
Just curious on this. What happens for any exceptions generated? I've found that when using the |
} | ||
|
||
pplx::task<http_response> asio_client::maybe_redirect(http_response response, http_request request, std::vector<uri> redirected_uris) { | ||
static const status_code found_status_code = 302; |
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 constant is already defined here: web::http::status_codes::Found
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.
Shouldnt we also react to:
- 301 Moved Permanently
- 307 Temporary Redirect
- 308 Permanent Redirect
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.
Yes (with the rules for only redirecting GET and HEAD taken into account). Also, the PR code comments refer to RFC 2068, which has been obsoleted twice-over, with RFC 7231 section 6.4 being the most recent spec I believe. This spec also removes the arbitrary maximum redirect count of 5 and explicitly recommends identifying cycles.
If this were to be added, it would be good to add to |
Specifically, config support could be made common for:
|
launch a new request with the redirection
5 redirects
Let me know if this approach is sound, if so then I will make tests.