tool_operate: keep failed partial download for retry auto-resume #18665
Conversation
b6ea6d0 to
b629c1e
Compare
src/tool_operate.c
Outdated
| (config->httpreq == TOOL_HTTPREQ_UNSPEC || | ||
| config->httpreq == TOOL_HTTPREQ_GET) && | ||
| (!config->customrequest || !strcmp(config->customrequest, "GET")) && | ||
| !config->use_ascii && !per->uploadfile && |
There was a problem hiding this comment.
use_ascii is documented to not have an effect on HTTP transfers. Why do we need to check that for resuming a retry?
Can uploadfile really be set if httpreq is GET or UNSPEC?
There was a problem hiding this comment.
use_ascii is documented to not have an effect on HTTP transfers. Why do we need to check that for resuming a retry?
I'll remove the check, after checking the code I don't think it's needed.
Note --use-ascii is documented for FTP and LDAP but from what I can see it sets CURLOPT_TRANSFERTEXT which may apply to more protocols in the library. And in the tool it looks like it leaves stdout in text mode no matter the protocol:
Lines 1292 to 1297 in 11b9912
Is this useful for Windows for other protocols? Like maybe HTTP something someone wants it converted to CRLF?
There was a problem hiding this comment.
Can uploadfile really be set if httpreq is GET or UNSPEC?
This I don't know for certain and I'd prefer to leave it for posterity.
src/tool_operate.c
Outdated
|
|
||
| if((scheme == proto_http || scheme == proto_https) && | ||
| method && !strcmp(method, "GET") && | ||
| response == (config->resume_from ? 206 : 200)) { |
There was a problem hiding this comment.
I would expect the server to also send Accept-Ranges: bytes to signal it accepts range requests (using byte ranges).
There was a problem hiding this comment.
I would expect the server to also send Accept-Ranges: bytes to signal it accepts range requests (using byte ranges).
Do most servers do this in practice? I'll add a check for it when response code is 200 (and not 206).
|
The CI complexity check is failing. Any advice for this? There's not much that could be considered complex except the conditional in the if statement, which I could move into a separate function but I'd prefer to leave it as is. |
|
I am trying a commit that moves the conditional to a separate function, but in my opinion it would be better inline |
|
Does anyone want to give this another look? I'd like to get it in before the feature freeze this weekend. |
- Keep data from a failed download instead of discarding it on retry in some limited cases when we know it's ok (currently only HTTP 200/206). Prior to this change on failed transfer the tool truncated any outfile data written before retrying the transfer. This change adds an exception for HTTP downloads when the user requested auto-resume, because in that case we can keep the outfile data and resume from the new position. Reported-by: tkzv@users.noreply.github.com Fixes curl#18035 Closes #xxxx
777d1d0 to
a7ef4fa
Compare
Prior to this change on failed transfer the tool truncated any outfile data written before retrying the transfer. This change adds an exception for HTTP downloads when the user requested auto-resume, because in that case we can keep the outfile data and resume from the new position.
Reported-by: tkzv@users.noreply.github.com
Fixes #18035
Closes #xxxx
This PR is a resurrection of #15333 which stalled