Skip to content
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

HTTP 429 after HTTP 303 redirect with --retry option retries original request location instead of new location #5462

Closed
kilork opened this issue May 26, 2020 · 11 comments

Comments

@kilork
Copy link

kilork commented May 26, 2020

I did this

POST /report/generate -> 303 Location /poll/<some id>
GET /poll/<some id> -> 429 with Retry-After or 301 with Location /download/<some id>

I am running the following curl command:

curl -v -L --retry 4 -H "Content-Type: application/json" --data '{"hello":"world"}' http://localhost:8000/report/generate

I want to see a single POST request to /report/generate, but instead I am getting first POST request to /report/generate then GET to /poll/..., which returns 429. In this moment it asks for some reason /report/generate, but this time with GET. I think this is a bug.

All logs and also sample server in deno I saved in gist here: https://gist.github.com/kilork/4899caddd6b6f9d9263c0b48c2d36a5c

I expected the following

First request is POST to /report/generate
Then come GET requests to /poll/
And finally I receive 301 to /download/

So basically I want to have poll until done single command. This works correctly with wget, but they do not proceed Retry-After, which also not quite pleasant.

curl/libcurl version

curl 7.70.0 (x86_64-w64-mingw32) libcurl/7.70.0 OpenSSL/1.1.1g (Schannel) zlib/1.2.11 brotli/1.0.7 libidn2/2.3.0 libpsl/0.21.0 (+libidn2/2.3.0) libssh2/1.9.0 nghttp2/1.40.0
Release-Date: 2020-04-29
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3 pop3s rtsp scp sftp smtp smtps telnet tftp
Features: AsynchDNS brotli HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz Metalink MultiSSL NTLM PSL SPNEGO SSL SSPI TLS-SRP

operating system

Windows 10

@kilork
Copy link
Author

kilork commented May 26, 2020

This #5237 seems to be also related, there is a long discussion about is it valid case. This issue looks like more complex scenario, where also retry involved.

@jay
Copy link
Member

jay commented May 26, 2020

I'd be more interested how we behave in the case of 301 (moved permanently). 303 (see other) is not supposed to be cached and there's no obligation to use the returned URI in future requests.

edit: If 303 then 301 I would think we retry the URI that led to 303. If just 301 I would think we retry the URI in the location header. I'm not 100% on this.

@kilork
Copy link
Author

kilork commented May 26, 2020

With 301 this was quite similar.

I think of HTTP client in this case as stateless, after 303 it cannot use anything which was not in this 303 request. And 303 is like recommendation to client, so it is not obligated to do anything about that. But in our case it is requesting this location from server, and the answer for this next request - 429. This is already not the original request, it does not make a lot of sense to repeat original request, because 429 is about another location in redirected request.

I would post here logs for 301 also.

@jay
Copy link
Member

jay commented May 27, 2020

POST /report/generate -> 301 /poll/1
GET /poll/1 -> 429 too many requests
(retry)
GET /report/generate -> 301 /poll/2

Looks like bugs to me. If the original URI should be used on retry then we should send the same request again with the original method. Another issue is when should the original URI be used. For example I would expect this:

POST /report/generate -> 301 /poll/1
GET /poll/1 -> 429 too many requests
(retry)
GET /poll/1 -> 429 too many requests
(retry)
GET /poll/1 -> 429 too many requests
(retry)
GET /poll/1 -> 301 /download/foo
GET /download/foo -> 429 too many requests
(retry)
GET /download/foo -> 429 too many requests
(retry)
GET /download/foo -> 200 ok

@kilork
Copy link
Author

kilork commented May 27, 2020

Yes, I started with 301, discovered that it does not work, but could also potentionally resend POST, which I do not want, then I red 303 description and was happy, but discovered this also does not work with current curl version. So I ended posting this issue here :) I would be happy with 301 without POST, seems like curl works this way, which also works for most cases. Just it can be an issue when it is not curl.

@bagder
Copy link
Member

bagder commented May 28, 2020

Looks like bugs to me. If the original URI should be used on retry then we should send the same request again with the original method.

I agree that this seems like a bug - and yes in this case curl should probably spot the redirect and do the retry on the redirected-to URL as I believe that's what this means. The retry isn't actually for the original POST.

bagder added a commit that referenced this issue Jun 1, 2020
When the method is updated inside libcurl we must still not change the
method as set by the user as then repeated transfers with that same
handle might not execute the same operation anymore!

This fixes the libcurl part of #5462

Test 1633 added to verify.
bagder added a commit that referenced this issue Jun 2, 2020
When the method is updated inside libcurl we must still not change the
method as set by the user as then repeated transfers with that same
handle might not execute the same operation anymore!

This fixes the libcurl part of #5462

Test 1633 added to verify.
@bagder
Copy link
Member

bagder commented Jun 2, 2020

With #5499, we change the behavior of libcurl to instead become like this:

POST /report/generate -> 301 /poll/1
GET /poll/1 -> 429 too many requests
(retry)
POST /report/generate -> 301 /poll/1
GET /poll/1 -> 429 too many requests
(retry)

i.e. since curl retries the exact same transfer again, it will do it from the POST. This is also probably not completely what we want, but I think the next step is to make curl (as opposed to libcurl) detect that there's been a POST => GET change and that there's a new URL for the retry so that the retry is done for that GET rather than to redo the POST.

@bagder
Copy link
Member

bagder commented Jun 2, 2020

Detecting that in curl is probably not possible without a libcurl feature that can tell what method that was used most recently, and since we're in feature-freeze that can't be merged until next feature window == after the pending release.

So, right now using -L together with --retry will mean that it retries the full thing if the http response code says so.

@kilork
Copy link
Author

kilork commented Jun 2, 2020

Thank you for your efforts! I also think it is a good idea to not rush into it.

bagder added a commit that referenced this issue Jun 2, 2020
When the method is updated inside libcurl we must still not change the
method as set by the user as then repeated transfers with that same
handle might not execute the same operation anymore!

This fixes the libcurl part of #5462

Test 1633 added to verify.

Closes #5499
@bagder
Copy link
Member

bagder commented Jun 12, 2020

I'm adding the remainder of this issue to the TODO document and will close this issue.

@bagder bagder closed this as completed in 95f2732 Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants