if curl fails, print a warning instead of exiting

[lesomnus]

Fix microsoft/vcpkg#32522

Currently, the download_files function is only used by binary caching providers (HttpGetBinaryProvider and GHABinaryProvider), so I think it is okay to change the function so that it does not exit with error.

I manually tested following scenarios:

--only-binarycaching	server up	server down
Without	OK	Continue with Warn
With	OK	Exit with Error

I couldn't find the relevant test code. If you let me know, I'll willing to write it.

[lesomnus]

I will add test about download_files at vcpkg-test/downloads.cpp tomorrow.

[BillyONeal]

I agree with your analysis on the impact of this for binary caching, but I do not agree with the solution you've chosen here. Just because today download_files is only used by binary caching does not mean that is all it will ever be used for. Moreover, this silences problems that probably should die like curl crashing.

Your bug report is that curl returned a valid exit code indicating a connection error, and we are interpreting that as 'curl crashed'. I agree that response is overblown, but treating real crashes as nothing isn't great either.

Perhaps having an allow list of known curl exit codes that indicate transient network issues should go into the 'retry' bucket?

[lesomnus]

@BillyONeal I thought that the program termination should be determined by the caller, not the download_files function. However, if curl returns an exit code such as 1 or 3 (unsupported protocol or malformed URL), it indicates a misconfiguration, and the user should be alerted about it. In this case, I agree with allowing the download_files function to terminate the program.

I will modify the code as you suggested: If three retries are attempted due to an exit code from the allowed list, and all of them fail, warning message is printed and response code of value 0 will returned (as in the current PR implementation). Do you have any opinions or suggestions regarding this?

[BillyONeal]

I thought that the program termination should be determined by the caller, not the download_files function.

Yes, I agree. I'm saying the interface needs to be changed to expose this error, not just eat the error while printing a warning.

I will modify the code as you suggested: If three retries are attempted due to an exit code from the allowed list, and all of them fail, warning message is printed and response code of value 0 will returned (as in the current PR implementation). Do you have any opinions or suggestions regarding this?

I think it needs to be exposed to the caller somehow so it can emit a reasonable message. Perhaps by returning std::vector<ExpectedL<int>> instead of std::vector<int>

[lesomnus]

The --retry N flag in curl retries the request N times in case of a transient error. However, the exit code 7 "Failed to connect to host" (microsoft/vcpkg#32522) is not considered as a transient error in curl, and this can be retried using the --retry-connrefused flag. But, retrying in the case of an actual server unavailability consumes significant time. Therefore, I did not add --retry-connrefused flag.

[lesomnus]

673dd2f failed the test since GitHub actions runner for ubuntu 20.04 has a lower version of curl (< 7.75.0, which does not support the %{exit_code} variable for --write-out).

I changed to determine errors based on HTTP response codes instead of the exit code. Also, considering the use of a lower version of curl, I assigned the command's exit code for the last error.

[autoantwort]

But, retrying in the case of an actual server unavailability consumes significant time.

With --retry 3 it's 7 seconds (1 + 2 + 4) which I think is ok.

In general I would vote for an option like --binarycache-hard-errors (See microsoft/vcpkg#28927)

[autoantwort]

I think the upload code has the same problem (exit code != 0 => exit vcpkg)

[xiaozhuai]

Any progress of this PR?
This is exactly the feature I need.

[BillyONeal]

Any progress of this PR? This is exactly the feature I need.

As written it breaks Ubuntu 20.04 customers so I don't think we could accept it in that condition

[lesomnus]

Any progress of this PR? This is exactly the feature I need.

As written it breaks Ubuntu 20.04 customers so I don't think we could accept it in that condition

My last comment means that I modified it by changing the way errors are determined (in 4daad5e) because my previous modifications failed in Ubuntu 20.04 (in commit 673dd2f).

[BillyONeal]

Interesting, with old curl or with how we do this today we don't get exit codes for anything in the 'middle'; I thought at least success or failure would be maintained correctly but nope:

[BillyONeal]

Sorry I broke the merge somehow, will fix...

[BillyONeal]

I was unable to get the merge to show up cleanly in GitHub, so I put everything into a squash commit. The SHA before I did this was 4daad5e

I also rewrote the status line parsing bit to avoid adding a dependency on iostreams.