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

Add Support for Retry in WP CLI Core Download Command #258

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

abhi3315
Copy link

@abhi3315 abhi3315 commented Apr 26, 2024

Description

This PR supports a retry mechanism in the WP CLI core download command. With the addition of --retry= and --retry-delay= flags, users can now specify the number of retry attempts and the delay between retries in case the download fails due to network issues or other transient errors.

Features Added:

  • The --retry=<retry> flag allows users to specify the number of retry attempts in case of download failure. The default value is set to 0.
  • The --retry-delay=<delay> flag allows users to specify the delay (in seconds) between retry attempts. The default delay is set to 5 seconds.

Usage Examples:

  • wp core download --retry=5 --retry-delay=10 - Downloads WordPress core with 5 retry attempts and a delay of 10 seconds between each attempt.
  • wp core download --retry=3 - Downloads WordPress core with 3 retry attempts using the default delay of 5 seconds between each attempt.

Issue: #155

@abhi3315 abhi3315 requested a review from a team as a code owner April 26, 2024 10:22
@@ -116,6 +116,12 @@ public function check_update( $_, $assoc_args ) {
* [--version=<version>]
* : Select which version you want to download. Accepts a version number, 'latest' or 'nightly'.
*
* [--retry=<retry>]
* : Number of retries in case of failure. Default is 0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think by default it can be 3. Since this retry functionality is in place, a default would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this part though. If it's just user opt-in, then makes sense to keep it 0?

if ( $md5_file === $md5_response->body ) {
WP_CLI::log( 'md5 hash verified: ' . $md5_file );
do {
$md5_response = Utils\http_request( 'GET', $download_url . '.md5', null, [], $options );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just discussed the retries with @schlessera and the Requests library does provide a way to hook options in curl - https://requests.ryanmccue.info/docs/hooks.html#:~:text=Requests%5Crequest_multiple().-,curl.before_request,-Set%20cURL%20options
#259

Not sure though if we should add it by default in Utils\http_request(). Thoughts @schlessera?

@schlessera
Copy link
Member

We do have a recurring need for retries in several places, so I think it would make sense to add the capability for retries to Utils\http_request(). It has an $options argument that we can extend to support retry semantics.

The only thing to keep aware of is that it already contains a retry mechanism for retrying failed verified requests in an unverified way. The logic will probably end up looking more complex that it should because of that.

*
* [--retry-delay=<retry-delay>]
* : Delay between retries in seconds. Default is 5 seconds.
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given our conversation on Zoom today, I think we should change this slightly.

  • Within wp core download, I think we should only have --retry=<count> as an argument. This will let us be opinionated about exponential backoff.
  • We can update Utils\http_request() to support 'retry' and 'retry_delay' as arguments.
  • The --retry=<count> argument should be applied to all HTTP requests within wp core download.

If WordPress.org is down in some manner, it's not helpful for WP-CLI users to keep hammering it with requests. Their systems should be tolerant to this fault. However, --retry=<count> with exponential (or linear) backoff (5 to ~30 seconds) and a maximum count seems like a good compromise.

We should have some tests!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielbachhuber As discussed in the call, we decided to wrap the whole try-catch block inside the do-while loop to handle the retry feature. But after taking a deeper look into it there are many early and exception throwing that may break the retry loop in between.

We should keep the retry loop in the download command only because updating the http_request util will affect other commands as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants