WordPress.org

Make WordPress Core

Opened 2 months ago

Last modified 40 hours ago

#54166 reopened defect (bug)

Updating errors due to PHP timeouts

Reported by: aristath Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: trunk
Component: Upgrade/Install Keywords: has-patch early needs-testing
Focuses: Cc:

Description

Initially reported in https://core.trac.wordpress.org/ticket/51857#comment:128
After [51815] there is an error when updating large plugins such as Gutenberg or WooCommerce.
After debugging the issue, it seems to be due to a PHP timeout and depends on the server's configuration (which is why the failure only occurs for large plugins).
The way we delete a directory in PHP is by going through all files in that directory and deleting them one by one (as far as I know this is the only way). As a result, deleting or restoring a backup of a large plugin takes more time than expected.

Change History (40)

This ticket was mentioned in PR #1696 on WordPress/wordpress-develop by aristath.


2 months ago

  • Keywords has-patch added; needs-patch removed

Fixes PHP timeouts by moving the backup deletion & restore to the shutdown action.
Processes running on shutdown are immune to PHP timeouts, so moving them there allows them to run _after_ the main process, without affecting the update.

Trac ticket: https://core.trac.wordpress.org/ticket/54166#ticket

#2 @SergeyBiryukov
2 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 months ago

#4 @prbot
2 months ago

peterwilsoncc commented on PR #1696:

I'm still hitting fatal errors occasionally during the update process.

In the screenshot it happened with Gutenberg, updating via "Dashboard > Updates > View details about... > Click install".

During testing last week it was happening consistently but now it's only happening intermittently, so this PR is an improvement.

https://i0.wp.com/user-images.githubusercontent.com/519727/135203206-4d79c87f-f776-42da-bf5b-d5c9178d5d24.png

#5 @peterwilsoncc
2 months ago

As noted on the pull request, I am still seeing fatal errors when testing this locally.

Both gutenberg and wpforms-lite intermittently end up with either some or all of their sub-directories empty.

There are backward compatibility implications but I am wondering if it's possible to switch to a series of AJAX requests for each step:

  1. Create the backup of the previous install
  2. Download the update from w.org
  3. Unzip the download
  4. Activate the plugin
  5. Restore if needs be

As fetch returns a promise, it's offers quite a lot of control to ensure the process doesn't get ahead of itself.

  1. No requirements
  2. No requirements
  3. Requires 1, 2 be complete
  4. Requires 1, 2, 3, be complete
  5. Requires 1 be complete, 4 to fail

#6 @peterwilsoncc
2 months ago

Please note: @aristath, @pbiron and I have some further notes relating to this issue on the original ticket from comment #137 onwards.

This ticket was mentioned in PR #1727 on WordPress/wordpress-develop by afragen.


2 months ago

Add a move_dir() function for use in WP_Upgrader instead of copy_dir(). There seem to be timeout issues in slower systems like Vagrant or the WP Docker test env with installation or update of large plugins like WooCommerce, Yoast SEO, or MailPoet.

This new function attempts a rename() and falls back to the previous copy_dir().

Trac ticket: https://core.trac.wordpress.org/ticket/54166

#8 @afragen
2 months ago

  • Keywords early dev-feedback added

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 months ago

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


2 months ago

#12 @afragen
2 months ago

  • Keywords needs-testing added

This ticket was mentioned in Slack in #core by afragen. View the logs.


8 weeks ago

#14 @SergeyBiryukov
8 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 51899:

Upgrade/Install: Introduce a move_dir() function.

This replaces the copy_dir() usage in WP_Upgrader::install_package() and aims to avoid PHP timeout issues when installing or updating large plugins on slower systems like Vagrant or the WP Docker test environment.

The new function attempts a native PHP rename() function first and falls back to the previous copy_dir().

Follow-up to [51815], [51898].

Props afragen, aristath, peterwilsoncc, galbaras, noisysocks, pbiron.
Fixes #54166. See #51857.

#15 @prbot
8 weeks ago

afragen commented on PR #1727:

Merged in r51899

#16 follow-up: @peterwilsoncc
8 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this as I think r51899 will need to be reverted:

  • rename() should not be in move_dir() -- different file systems use different functions, eg ftp_rename()
  • If the destination exists, it is always overwritten. This seems problematic and there should really be an $overwrite parameter per the other functions
  • In _wp_handle_upload() it's noted that rename() fails on streams, so that may be problematic & streams handled differently ('may' is the key word, with the other issues I haven't researched this)

When trying to debug this issue with @afragen on Friday, I was testing with the pull request applied and still having the problem so I am not sure it fixes the reported issue completely either.


Reading the code and doing some logging earlier today, I think something that runs asynchronously isn't been accounted for which is leading to the error.

Installing/updating akismet (a very small plugin) was showing different contents within the working directory at several points within the procedure.

#17 @afragen
8 weeks ago

Just for reference, if rename() fails the function reverts to copy_dir(), which was there previously. File systems not having a rename(j command should continue to function as they did previously.

The previous flow always emptied the specific plugin folder in wp-content/plugins/ that's why a failed update would leave an empty folder. The $clear_destination parameter is always true when updating. https://github.com/WordPress/wordpress-develop/blob/6367c01844e34dd372b999beb0e370f52d8c42d5/src/wp-admin/includes/class-plugin-upgrader.php#L222-L239

I don't believe streams are ever utilized in the plugin update process.

While Peter and I haven't yet been able to figure out the issue on his system using Vagrant and Chassis, I don't believe that reverting r51899 will fix it.

Version 0, edited 8 weeks ago by afragen (next)

#18 @SergeyBiryukov
8 weeks ago

In a discussion with @aristath today, it was pointed out that PR 1696 is also ready for commit, so let's try that as well.

#19 @SergeyBiryukov
7 weeks ago

In 51902:

Upgrade/Install: Restore or clean up the temporary plugin or theme backup on shutdown.

This allows these actions to run after the main process, without affecting the update. Actions running on shutdown are immune to PHP timeouts, so in case the failure was due to a PHP timeout, we'll still be able to properly restore the previous version.

Follow-up to [51815], [51898], [51899].

Props aristath, peterwilsoncc.
See #54166.

#20 in reply to: ↑ 16 ; follow-up: @SergeyBiryukov
7 weeks ago

Replying to peterwilsoncc:

Reopening this as I think r51899 will need to be reverted

Yeah, now that I think of it, I also have some concerns with the function:

  • It does not use the WP_Filesystem_* methods, so the rename() call will only work if direct PHP file access is available.
  • The fallback to copy_dir() causes inconsistent behavior:
    • If the rename() call worked, the source directory is successfully renamed and does not need any further actions.
    • If the rename() call failed, the source directory is left behind and has to be cleaned separately.

I wonder [51899] is still needed after [51902], or after getting to the bottom of the issue from comment:16.

This ticket was mentioned in PR #1747 on WordPress/wordpress-develop by afragen.


7 weeks ago

Update for r51899 to address issues, https://core.trac.wordpress.org/ticket/54166#comment:20

  • Add check for direct PHP flle access and only use rename() if true.
  • More consistent behavior for fallback to copy_dir()

Trac ticket: https://core.trac.wordpress.org/ticket/54166

#23 @prbot
7 weeks ago

afragen commented on PR #1747:

Use move_dir() for moving and restoring to backup directory as $wp_filesystem->move() fallback is a file copy and that will cause problems.

#24 in reply to: ↑ 20 ; follow-up: @afragen
7 weeks ago

Replying to SergeyBiryukov:

Replying to peterwilsoncc:

Reopening this as I think r51899 will need to be reverted

Yeah, now that I think of it, I also have some concerns with the function:

  • It does not use the WP_Filesystem_* methods, so the rename() call will only work if direct PHP file access is available.
  • The fallback to copy_dir() causes inconsistent behavior:
    • If the rename() call worked, the source directory is successfully renamed and does not need any further actions.
    • If the rename() call failed, the source directory is left behind and has to be cleaned separately.

I wonder [51899] is still needed after [51902], or after getting to the bottom of the issue from comment:16.

I think I have addressed the above concerns in a new PR#1747

I believe move_dir() is still needed it does address a potential timeout issue and does create a directory move that doesn't fallback to a PHP copy() that may fail when attempting to copy directories.

#25 in reply to: ↑ 24 @SergeyBiryukov
7 weeks ago

Replying to afragen:

Yeah, now that I think of it, I also have some concerns with the function:

  • It does not use the WP_Filesystem_* methods, so the rename() call will only work if direct PHP file access is available.
  • The fallback to copy_dir() causes inconsistent behavior:
    • If the rename() call worked, the source directory is successfully renamed and does not need any further actions.
    • If the rename() call failed, the source directory is left behind and has to be cleaned separately.

I wonder [51899] is still needed after [51902], or after getting to the bottom of the issue from comment:16.

I think I have addressed the above concerns in a new PR#1747

Thanks for the PR! I think it addresses the first concern, but not the second. After the fallback to copy_dir(), I think the source directory should be removed. Currently, in that scenario, move_dir() is just an alias for copy_dir(), as it performs the copying part, but does not clean up the source directory like the move_dir() name suggests, unless I'm missing something.

Addressing that would make the clear_working parameter of WP_Upgrader::install_package() a bit redundant, as the working directory would always be cleared regardless of its value. However, it is already redundant in case the rename() call worked. If move_dir() stays, the clear_working parameter would probably have to be deprecated.

#26 @prbot
7 weeks ago

afragen commented on PR #1747:

Updated to clear the working directory so there is internal parity within the function with the results of a successful rename().

As @SergeyBiryukov points out, this may deprecate the clear_working option.

#27 @afragen
7 weeks ago

  • Keywords dev-feedback removed

This ticket was mentioned in Slack in #core by sergey. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core-auto-updates by peterwilsoncc. View the logs.


3 weeks ago

#30 @peterwilsoncc
3 weeks ago

Please see my full comment on the original ticket.

I've come to the conclusion that the following commits need to be reverted and the reliability improvements added in a subsequent release:

Repeating the tl;dr here but let's discuss pros and cons on the other ticket.

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


2 weeks ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


2 weeks ago

#33 @afragen
2 weeks ago

@SergeyBiryukov we should commit the rest of this PR for 5.9?

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


9 days ago

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 days ago

This ticket was mentioned in Slack in #core-auto-updates by hellofromtonya. View the logs.


2 days ago

#37 @SergeyBiryukov
2 days ago

In 52289:

Upgrade/Install: Make some adjustments to the move_dir() function:

  • Check for direct PHP flle access and only use rename() if true.
  • Check whether the destination directory was successfully created.
  • Clear the working directory so there is internal parity within the function between the results of a successful rename() and a fallback to copy_dir().
  • Use move_dir() in WP_Upgrader::move_to_temp_backup_dir() and ::restore_temp_backup().

Follow-up to [51815], [51898], [51899], [51902], [52192], [52284].

Props afragen, peterwilsoncc, dd32, SergeyBiryukov.
See #54166, #51857.

#38 @SergeyBiryukov
2 days ago

Thanks everyone! Keeping this open for now to see if the clear_working parameter is still relevant or should be deprecated, as per comment:25.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 days ago

Note: See TracTickets for help on using tickets.