Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#54546 new defect (bug)

Fatal error receive while updating WP 5.8.2 to WP 5.9.

Reported by: apeksha10's profile apeksha10 Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.9
Component: Upgrade/Install Keywords: needs-testing has-patch dev-feedback
Focuses: Cc:

Description (last modified by pbiron)

After updating to 5.9-beta1 via the Beta Tester plugin, I got the below error

https://content.screencast.com/users/ApekshaShah/folders/Capture/media/8b2bed1a-ca74-4ebc-83c0-c2bb7d0c0eb3/LWR_Recording.png

Fatal error: Uncaught Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

I also updated a few other sites on my local machine and I'm getting that error on the other sites.

Full information:

Unpacking the update...
Verifying the unpacked files...
Preparing to install the latest version...
Enabling Maintenance mode...
Copying the required files...
Disabling Maintenance mode...
Upgrading database...

Fatal error: Uncaught Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

Call Stack:

# Function Location
1 {main}{} ..\update-core.php:0
2 do_core_upgrade() ..\update-core.php:1106
3 Core_Upgrader->upgrade() ..\update-core.php:887
4 update_core() ..\class-core-upgrader.php:172
5 wp_remote_post() ..\update-core.php:1409
6 WP_Http->post() ..\http.php:179
7 WP_Http->request() ..\http.php:608
8 Requests->request() ..\class-http.php:394
9 Requests_Transport_cURL->request() ..\class-requests.php:381
10 Requests_Transport_cURL->process_response() ..\cURL.php:179

Change History (56)

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


3 years ago

#2 @SergeyBiryukov
3 years ago

  • Component changed from General to Upgrade/Install
  • Milestone changed from Awaiting Review to 5.9
  • Version set to trunk

Hi there, welcome to WordPress Trac! Thanks for the report.

Related: #54547. Moving to the milestone for more visibility.

#3 @hellofromTonya
3 years ago

  • Description modified (diff)

#4 follow-ups: @hellofromTonya
3 years ago

  • Keywords reporter-feedback added

Requests library:

  • Requests pre-2.0.0:
    • the "cURL" class was called Requests_Transport_cURL and was in a file named wp-includes/Requests/Transport/cURL.php.
    • Line 443 in cURL.php file throws a new Requests_Exception() error
  • Requests 2.0.0:
    • the "cURL" class was renamed to WpOrg\Requests\Transport\Curl and its file renamed to wp-includes/Requests/Transport/Curl.php.
    • Requests_Exception class was removed in this version
    • The cURL error was changed to throw a new Exception

[52244] #54504 updated the Requests library to 2.0.0.

Look at the fatal error:

Fatal error: Uncaught Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

It's indicating the error happened in cURL.php file on line 443. That file was renamed in Requests 2.0.0 and line 443 is from the older version, not the new version (i.e. in 5.9.0 Beta 1).

Look at the 2nd error:

Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

That class was removed in newer version (i.e. in 5.9.0 Beta 1).

At Upgrading database... the old files should have been removed and new files installed. Did this happen?

@apeksha10 if you're able to, can you check if the cURL file is named cURL.php or Curl.php? Look here: C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\Curl.php. Is it named cURL.php or Curl.php?

Last edited 3 years ago by SergeyBiryukov (previous) (diff)

#5 @hellofromTonya
3 years ago

@costdev reported on Slack the same problem. Copying the message here.

  • After clicking "Update", it takes ages (to be exact) for the "Update WordPress" page to load and show the steps for the update process.
  • As soon as it finally shows the page, my debug.log appeared with:
PHP Fatal error:  Uncaught Error: Class 'Requests_Exception' not found in <path>\wp-includes\Requests\Transport\cURL.php:422
Stack trace:
#0 <path>\wp-includes\Requests\Transport\cURL.php(177): Requests_Transport_cURL->process_response('', Array)
#1 <path>\wp-includes\class-requests.php(379): Requests_Transport_cURL->request('http://...', Array, NULL, Array)
#2 <path>\wp-includes\class-http.php(370): Requests::request('http://...', Array, NULL, 'POST', Array)
#3 <path>\wp-includes\class-http.php(589): WP_Http->request('http://...', Array)
#4 <path>\wp-includes\http.php(187): WP_Http->post('http://...', Array)
#5 <path>\wp-admin\includes\update-core.php(1409): wp_remote_post('http in <path>\wp-includes\Requests\Transport\cURL.php on line 422

In my case, I was upgrading from 5.0.14 > 5.9.0 Beta 1.

@costdev can you check if the cURL file is named cURL.php or Curl.php (see my comments above?

#6 follow-up: @costdev
3 years ago

The cURL file is named cURL.php.

#7 @hellofromTonya
3 years ago

A similar report is found in Support Forum https://wordpress.org/support/topic/fatal-error-during-update-to-5-9-beta-1/. Copying the information here.

---

Got a fatal error during update from 5.8.2 to 5.9 beta 1 via Beta Tester plugin. Here it is:

Fatal error: Uncaught Error: Class "Requests_Exception" not found in \wp-includes\Requests\Transport\cURL.php:443

Stack Trace
1.	
Requests_Transport_cURL->process_response('', Array)
\wp-includes\Requests\Transport\cURL.php:179
2.	
Requests_Transport_cURL->request('https://test...', Array, NULL, Array)
\wp-includes\class-requests.php:381
3.	
Requests::request('https://test...', Array, NULL, 'POST', Array)
\wp-includes\class-http.php:394
4.	
WP_Http->request('https://test...', Array)
\wp-includes\class-http.php:608
5.	
WP_Http->post('https://test...', Array)
\wp-includes\http.php:179
6.	
wp_remote_post('https://test...', Array)
\wp-admin\includes\update-core.php:1409
7.	
update_core('C:/Programs/lar...', 'C:/Programs/lar...')
\wp-admin\includes\class-core-upgrader.php:172
8.	
Core_Upgrader->upgrade(Object(stdClass), Array)
\wp-admin\update-core.php:877
9.	
do_core_upgrade(false)
\wp-admin\update-core.php:1106
10.	
{main}
thrown in \wp-includes\Requests\Transport\cURL.php on line 443

For the local environment I am using Laragon and Windows 10. And I can see Exception.php in Requests folder.

---

#8 in reply to: ↑ 6 @hellofromTonya
3 years ago

Replying to costdev:

The cURL file is named cURL.php.

Thanks @costdev.

Please open that file. What's the class name and is it namespaced WpOrg\Requests\Transport?

Also, open the Requests\Exception.php file. What's the class name and is it namespaced WpOrg\Requests\?

Last edited 3 years ago by hellofromTonya (previous) (diff)

#9 @costdev
3 years ago

wp-includes/Requests/Transport/cURL.php:

<?php

namespace WpOrg\Requests\Transport;

...

final class Curl implements Transport {

wp-includes/Requests/Exception.php:

<?php

namespace WpOrg\Requests;

...

class Exception extends PHPException {

#10 in reply to: ↑ 4 ; follow-up: @jrf
3 years ago

Replying to hellofromTonya:

Requests library:

  • Requests pre-2.0.0:
    • the "cURL" class was called Requests_Transport_cURL and was in a file named wp-includes/Requests/Transport/cURL.php.
    • Line 443 in cURL.php file throws a new Requests_Exception() error
  • Requests 2.0.0:
    • the "cURL" class was renamed to WpOrg\Requests\Transport\Curl and its file renamed to wp-includes/Requests/Transport/Curl.php.
    • Requests_Exception class was removed in this version
    • The cURL error was changed to throw a new Exception

Just making sure the base information is correct:

Requests_Exception was not removed. It became namespaced, just like every other class. The file was not renamed/moved as it was already called Exception.php.

So the Exception you see being thrown in the Curl class is still the same exception, just namespaced now (and imported via a use WpOrg\Requests\Exception import statement).


As for the issues you are seeing, yes, I agree, these are likely to do with file name casing and that not being taken into account properly during the upgrade.

In the Requests repo itself, we moved all files. The "old" PSR-0 classes lived in a library directory, the new PSR-4 based classes live in a src directory.
That prevents these kind of issues from happening as the files are seen as "new" files.

Unfortunately, WP Core had nearly* all the files in the wp-includes/requests directory, not in wp-includes/requests/libary, so that didn't really allow for making the same directory move.

Would moving the files to wp-includes/requests/src be a potential solution ? (to make sure they are seen as new files, not updated files)

* One exception to this: the original entry point class Requests was in WP Core in the wp-includes/class-requests.php file (in Requests that class was in library/Requests.php).
In Requests 2.0 a stripped down version of the "old" library/Requests.php file still remains in place as part of the BC-layer.
The actual functional code, however, lives in the "new" src/Requests.php file.
For WP Core, the wp-includes/class-requests.php file now contains the stripped down version of the library/Requests.php file and along the same lines as in Requests itself, the real functional code now lives in the wp-includes/requests/Requests.php file.

#11 follow-up: @hellofromTonya
3 years ago

Running more tests with @costdev. Let me share findings so far.

My Machine (macOS)

On my machine:

  • localhost: Local
  • OS: macOS
  • Plugins: Core Rollback and Beta Tester - both are activated
  • Browser: Edge

Upgrading from 5.8.2 to 5.9.0 Beta 1 via the Beta Tester

  • No errors and nothing in the logs
  • Upgrade is successful
  • Requests library is updated with the newest 2.0.0 code
  • But the renamed files are not renamed:
    • wp-includes/Requests/Iri.php is not renamed to wp-includes/Requests/Iri.php
    • wp-includes/Requests/Transport/cURL.php is not renamed to wp-includes/Requests/Transport/Curl.php
  • Site Health's Filesystem Permissions show:
    The main WordPress directory	Writable
    The wp-content directory	Writable
    The uploads directory	        Writable
    The plugins directory	        Writable
    The themes directory	        Writable
    

Downgrading from 5.9.0 Beta 1 to 5.8.2 via the Beta Tester:

  • The ugly orange PHP error interface flashes
  • Upgrade is successful
  • Requests library is restored to the older version
  • Filenames mentioned above remain, which matches the 5.8 version
  • But debug.log has the following warning:
[01-Dec-2021 17:21:49 UTC] PHP Warning:  rename(/var/tmp/wordpress-5.8.2-no-content-gTRqga.tmp,wordpress-5.8.2-no-content.zip): Operation not permitted in ../Local/test/app/public/wp-admin/includes/file.php on line 1201
[01-Dec-2021 17:21:49 UTC] PHP Stack trace:
[01-Dec-2021 17:21:49 UTC] PHP   1. {main}() ../Local/test/app/public/wp-admin/update-core.php:0
[01-Dec-2021 17:21:49 UTC] PHP   2. do_core_upgrade() ../Local/test/app/public/wp-admin/update-core.php:1108
[01-Dec-2021 17:21:49 UTC] PHP   3. Core_Upgrader->upgrade() ../Local/test/app/public/wp-admin/update-core.php:879
[01-Dec-2021 17:21:49 UTC] PHP   4. Core_Upgrader->download_package() ../Local/test/app/public/wp-admin/includes/class-core-upgrader.php:124
[01-Dec-2021 17:21:49 UTC] PHP   5. download_url() ../Local/test/app/public/wp-admin/includes/class-wp-upgrader.php:309
[01-Dec-2021 17:21:49 UTC] PHP   6. rename() ../Local/test/app/public/wp-admin/includes/file.php:1201
  • Site Health's Filesystem Permissions show:
The main WordPress directory	Writable
The wp-content directory	Writable
The uploads directory	        Writable
The plugins directory	        Writable
The themes directory	        Writable

So the filesystem is the same as Windows, i.e. meaning the file name case does not seem to be an issue. Files are loading properly.

Colin's machine (Windows)

On my machine:

  • localhost: Local
  • OS: Windows
  • Plugins: Beta Tester
  • Browser: Edge

Upgrading from 5.0.14 to 5.9.0 Beta 1

  • Fatal error:
PHP Fatal error:  Uncaught Error: Class 'Requests_Exception' not found in \wp-includes\Requests\Transport\cURL.php:422
Stack trace:
#0 \wp-includes\Requests\Transport\cURL.php(177): Requests_Transport_cURL->process_response('', Array)
#1 \wp-includes\class-requests.php(379): Requests_Transport_cURL->request('http://...', Array, NULL, Array)
#2 \wp-includes\class-http.php(370): Requests::request('http://...', Array, NULL, 'POST', Array)
#3 \wp-includes\class-http.php(589): WP_Http->request('http://...', Array)
#4 \wp-includes\http.php(187): WP_Http->post('http://...', Array)
#5 \wp-admin\includes\update-core.php(1409): wp_remote_post('http://...', Arra in \wp-includes\Requests\Transport\cURL.php on line 422
  • Files are upgraded with the new code.
  • Like my machine, the IRI.php and cURL.php files were not renamed.

Downgrading from 5.9.0 Beta 1 to 5.8.2

  • Error:
Uncaught Error: Class 'WpOrg\Requests\Exception' not found in \wp-includes\Requests\Transport\cURL.php:492
Stack trace:
#0 \wp-includes\Requests\Transport\cURL.php(218): WpOrg\Requests\Transport\Curl->process_response('', Array)
#1 \wp-includes\Requests\Requests.php(468): WpOrg\Requests\Transport\Curl->request('http://...', Array, '', Array)
#2 \wp-includes\class-wp-http.php(394): WpOrg\Requests\Requests::request('http://...', Array, NULL, 'POST', Array)
#3 \wp-includes\class-wp-http.php(614): WP_Http->request('http://...', Array)
#4 \wp-includes\http.php(179): WP_Http->post('http://...', Array)
#5 \wp-admin\includes\update-core.php(1408): w in \wp-includes\Requests\Transport\cURL.php on line 492
  • Files are upgraded with the 5.8.2 code
  • File names remain (as they should)

What's interesting though is look at the code that's being executed in the stack trace on his machine:

👉 It's the "from" code not the "to" code.

The code in memory is the "from" code whereas the filesystem has been upgraded with the "to" code. As the exception file is not loaded into memory before upgrading the files on the filesystem, when the cURL error happens, it attempts to throw an exception for a class that does not exist.

Q: What cURL error is happening on Windows machines?

Colin tracked it to:

cURL error 28: Operation timed out after 60006 milliseconds with 0 bytes received 

Q: Is the "from" code running instead of the "to" code for the Upgrade process?

WordPress 5.9 is incompatible with Gutenberg plugin less than 11.9. So if Gutenberg plugin version 11.8 is installed, it should get deactivated during the upgrade process to WordPress 5.9.

Steps to test:

  1. Go to the Gutenberg plugin advanced page and download 11.8.3.
  2. Install and activate it.
  3. Start with WordPress 5.7.4 (use the Core Rollback plugin)
  4. Then upgrade via Beta Tester to WordPress 5.9.0 beta 1.
  5. Go to Plugins and check that Gutenberg plugin was deactivated.

Test Results:

  • My machine: Yes, the older Gutenberg plugin gets deactivated. That code is in 5.9.0 not in 5.8.2 or 5.7.4.
  • Colin's machine: same results

So yes, the 5.9 upgrader code is executing.

Q: Why then is the "from" version code running instead of the "to" version code?

Is the "from" version code being loaded into memory before the upgrade process starts?

Looking at above at Colin's results, it seems to be the case for the HTTP and Requests code. Since the "from" version code is running, when the cURL timeout happens, it attempts to throw an exception that hasn't yet been loaded into memory. But the filesystem is now different and doesn't have that class. Fatal error.

Is this only on Windows? Or is this true for all systems?

#12 in reply to: ↑ 11 ; follow-ups: @SergeyBiryukov
3 years ago

Replying to hellofromTonya:

Q: Is the "from" code running instead of the "to" code for the Upgrade process?

WordPress 5.9 is incompatible with Gutenberg plugin less than 11.9. So if Gutenberg plugin version 11.8 is installed, it should get deactivated during the upgrade process to WordPress 5.9.

Hmm, unless I'm missing something, I don't think the Gutenberg plugin is relevant here, but rather the fact that the updater itself uses an older version of the Requests library during the core update process.

Replying to jrf:

Would moving the files to wp-includes/requests/src be a potential solution ? (to make sure they are seen as new files, not updated files)

Good question, let's try that :)

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


3 years ago

#14 @costdev
3 years ago

Hmm, unless I'm missing something, I don't think the Gutenberg plugin is relevant here, but rather the fact that the updater itself uses an older version of the Requests library during the core update process.

This was a test to find out whether the "from" code or the "to" code was being used during the upgrade process.

Going from 5.7.4 to 5.9.0 Beta 1:

  • If it was the "from" code, Gutenberg would stay activated as 5.7.4 supports Gutenberg < 11.9.
  • If it was the "to" code, Gutenberg would be deactivated as WP 5.9+ doesn't support Gutenberg < 11.9.

When Gutenberg was deactivated, this told us that it's the "to" code that is used during the upgrade process.

Last edited 3 years ago by costdev (previous) (diff)

#15 in reply to: ↑ 12 ; follow-up: @hellofromTonya
3 years ago

Replying to SergeyBiryukov:

Hmm, unless I'm missing something, I don't think the Gutenberg plugin is relevant here, but rather the fact that the updater itself uses an older version of the Requests library during the core update process.

Yes that is what is expected to happen. The point of the test was to validate that the "to" (i.e. 5.9.0) wp-admin/includes/update-core.php was loaded into memory and being executed. The fact that the older version of Gutenberg was deactivated proves that 5.9's version of update-core.php was the file being executed.

Why did we do that check? Because the older versions of HTTP and Requests were the ones being executed in memory. We needed to figure out if the Updater was the older (i.e. the "from" version) or the new 5.9.0 version.

Last edited 3 years ago by hellofromTonya (previous) (diff)

#16 in reply to: ↑ 12 @jrf
3 years ago

Thanks @hellofromTonya and @costdev for your investigation efforts so far!

How I read it, there are two (three) separate, distinct issues in play (previously undiscovered bugs), which just happen to both be triggered during this update:

1. The upgrade process does not isolate itself enough from the files being updated.

What I mean by that is, that any essential WP Core files (potentially) being used during the upgrade, should be loaded into memory (require-d) prior to the update starting.

This is clearly not happening as otherwise, the Fatal for the Requests_Exception would not be thrown, which means that you could end up with a mix of "old" and "new" files being used during an upgrade, which can cause race conditions.

For the majority of code in Core, this will not happen, as files are hard require-d and not being lazily autoloaded. But for external dependencies, like Requests, where an autoloader is used, this is a realistic risk and should be mitigated.

While the "new" upgrader code should always be used, the rest of the files should be consistently from the "old" codebase.

To fix this, some investigation would be needed to figure out exactly which files/parts of Core are used during the upgrade.
Once this is known, any files on that list which depend on an autoloader, should be hard-required.
A DirectoryIterator with require_once statements for those parts of Core, could probably solve this in a future-proof way.

2. The upgrade process is not designed to handle file renames involving only the file/directory casing.

Whether or not this causes problems will highly depend on the OS and whether or not the file-system is case-sensitive.

To solve this, it could be considered to unlink old files and then move the new files in place, instead of plain moving the files (replacing the original files).

I'm by no means saying that these are the solutions to use. Just pointing out some solution directions to explore.

Once these two issues are solved, we can start to try and figure out why the Curl timeout is happening.... (problem three)

#17 in reply to: ↑ 15 ; follow-up: @SergeyBiryukov
3 years ago

Replying to hellofromTonya:

Yes that is what is expected to happen. The point of the test was to validate that the "to" (i.e. 5.9.0) wp-admin/includes/update-core.php was loaded into memory and being executed. The fact that the older version of Gutenberg was deactivated proves that 5.9's version of update-core.php was the file being executed.

Why did we do that check? Because the older versions of HTTP and Requests were the ones being executed in memory. We needed to figure out if the Updater was the older (i.e. the "from" version) or the new 5.9.0 version.

Ah, that's what I was missing. Thanks for the clarification!

I can confirm that wp-admin/includes/update-core.php is indeed specifically copied from the new package, however the rest of the code, including the Requests library, appears to be loaded from the current (older) install.

#18 in reply to: ↑ 4 ; follow-up: @apeksha10
3 years ago

Replying to hellofromTonya:

Requests library:

  • Requests pre-2.0.0:
    • the "cURL" class was called Requests_Transport_cURL and was in a file named wp-includes/Requests/Transport/cURL.php.
    • Line 443 in cURL.php file throws a new Requests_Exception() error
  • Requests 2.0.0:
    • the "cURL" class was renamed to WpOrg\Requests\Transport\Curl and its file renamed to wp-includes/Requests/Transport/Curl.php.
    • Requests_Exception class was removed in this version
    • The cURL error was changed to throw a new Exception

[52244] #54504 updated the Requests library to 2.0.0.

Look at the fatal error:

Fatal error: Uncaught Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

It's indicating the error happened in cURL.php file on line 443. That file was renamed in Requests 2.0.0 and line 443 is from the older version, not the new version (i.e. in 5.9.0 Beta 1).

Look at the 2nd error:

Error: Class 'Requests_Exception' not found in C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\cURL.php on line 443

That class was removed in newer version (i.e. in 5.9.0 Beta 1).

At Upgrading database... the old files should have been removed and new files installed. Did this happen?

@apeksha10 if you're able to, can you check if the cURL file is named cURL.php or Curl.php? Look here: C:\Users\User\Local Sites\first-localhost\app\public\wp-includes\Requests\Transport\Curl.php. Is it named cURL.php or Curl.php?

The file name is Curl.php [Curl.php]

https://www.screencast.com/t/BWNZ2gfUsE

#19 in reply to: ↑ 18 @hellofromTonya
3 years ago

  • Keywords reporter-feedback removed

Replying to apeksha10:

The file name is Curl.php [Curl.php]

https://www.screencast.com/t/BWNZ2gfUsE

Interesting. Thank you for confirming!

#20 in reply to: ↑ 17 ; follow-up: @hellofromTonya
3 years ago

  • Keywords needs-patch needs-testing added

Replying to SergeyBiryukov:

I can confirm that wp-admin/includes/update-core.php is indeed specifically copied from the new package, however the rest of the code, including the Requests library, appears to be loaded from the current (older) install.

Thank you Sergey for confirming.

What's Known

What's known about the upgrade process:

  • The "from" version of the Core code is loaded into memory and is running, including HTTP and Requests
  • The "to" version of the wp-admin/includes/update-core.php code is running as it's injected/copied to run (makes sense)
  • But the filesystem no longer has the "from" version files available, meaning if some file was not loaded into memory before update_core( $from, $to ) runs (i.e. which is in the "to" version of the the wp-admin/includes/update-core.php file), then a fatal error can happen
  • And a cURL timeout error happens which so far is isolated to only Windows OS
  • And because an error happens, to "to" version of the exception file isn't loaded into memory causing the fatal error

The 2 Known Problems

There are 2 problems surfaced from this ticket:

  1. Problem 1: Fatal error. Root cause reason is known.
    • Root cause reason: Not all of the needed upgrade code is loaded into memory before the filesystem is changed with the new "to" version of the files.
    • Note: In this specific case, it's the exception file within Requests. But it could easily be some other code for a different or future scenario.
  2. Problem 2: cURL timeout error happens. Root cause reason is unknown.
    • Root cause reason: ??

Solving Problem 1

Problem 1 is solvable by doing what @jrf recommended: identify what files are needed for the upgrade and then get them loaded into memory before changing the filesystem. This can happen in the "to" version of the wp-admin/includes/update-core.php file by:

  • Identifying which "from" version files needed to be loaded.
  • Doing a require_once to load each of those files at the top of update_core(), i.e. before delete and copy filesystem code.

I'm adding needs-patch and needs-testing for fixing this problem.

Solving Problem 2

Ideas?

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


3 years ago

#22 in reply to: ↑ 10 ; follow-up: @pbiron
3 years ago

Replying to jrf:

Just making sure the base information is correct:

Requests_Exception was not removed. It became namespaced, just like every other class. The file was not renamed/moved as it was already called Exception.php.

So the Exception you see being thrown in the Curl class is still the same exception, just namespaced now (and imported via a use WpOrg\Requests\Exception import statement).

Sorry if this has been mentioned before (haven't had time to read all the comments on this ticket), but...

Yes, the file is still there, but the class defined in that file changed from the unnamespaced Request_Exception to simple Exception in the new namespace.

#23 in reply to: ↑ 20 @pbiron
3 years ago

Replying to hellofromTonya:

The 2 Known Problems

There are 2 problems surfaced from this ticket:

  1. Problem 1: Fatal error. Root cause reason is known.
    • Root cause reason: Not all of the needed upgrade code is loaded into memory before the filesystem is changed with the new "to" version of the files.
    • Note: In this specific case, it's the exception file within Requests. But it could easily be some other code for a different or future scenario.
  2. Problem 2: cURL timeout error happens. Root cause reason is unknown.
    • Root cause reason: ??

Has anyone been able to identify what is making the "Problem 2" request? And what is being requested? Is it the updater itself (e.g., trying to update language packs after core has been updated)? Or is it possibly a plugin that has hooked into upgrader_process_complete?

#24 @costdev
3 years ago

@pbiron Sorry, I meant to post this earlier today:

I added some logging and got this just before the fatal occurs:

Error: cURL error 28: Operation timed out after 60002 milliseconds with 0 bytes received
curl_getinfo:
Array
(
    [url] => http://582.local/wp-admin/upgrade.php?step=upgrade_db
    [content_type] => 
    [http_code] => 0
    [header_size] => 0
    [request_size] => 262
    [filetime] => -1
    [ssl_verify_result] => 0
    [redirect_count] => 0
    [total_time] => 60.002039
    [namelookup_time] => 0.000425
    [connect_time] => 0.000625
    [pretransfer_time] => 0.00065
    [size_upload] => 0
    [size_download] => 0
    [speed_download] => 0
    [speed_upload] => 0
    [download_content_length] => -1
    [upload_content_length] => 0
    [starttransfer_time] => 0
    [redirect_time] => 0
    [redirect_url] => 
    [primary_ip] => ::1
    [certinfo] => Array
        (
        )

    [primary_port] => 80
    [local_ip] => ::1
    [local_port] => 57417
    [http_version] => 0
    [protocol] => 1
    [ssl_verifyresult] => 0
    [scheme] => HTTP
    [appconnect_time_us] => 0
    [connect_time_us] => 625
    [namelookup_time_us] => 425
    [pretransfer_time_us] => 650
    [redirect_time_us] => 0
    [starttransfer_time_us] => 0
    [total_time_us] => 60002039
)

#25 in reply to: ↑ 22 @hellofromTonya
3 years ago

Replying to pbiron:

Yes, the file is still there, but the class defined in that file changed from the unnamespaced Request_Exception to simple Exception in the new namespace.

Yes, the file is still there. However, it has not been loaded into memory. The "from" version code is in memory including the autoloader. But the filesystem has copied changed the code. So for this specific issue, the autoloader in Requests loads the "to" version of the exception file into memory, but the code in that file is the "to" version which does not match the needs of the "from" version. It's the wrong class. Mismatch of "from" and "to" versions of the code.

This is why the solution proposed for Problem 1 is to preload all files that are potentially needed for the upgrade process before changing the filesystem files/code. This extra step would ensure all needed code is already in memory so that no additional files are needed to be loaded, such as the exception file in the Requests library.

Does that make sense @pbiron?

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


3 years ago

#27 @costdev
3 years ago

Update on Fatal Error

The Cause

  • The Requests_Exception class isn't loaded into memory prior to the upgrade running.
  • When Requests 2.0 is downloaded and copied, it overwrites the contents of wp-includes/Requests/Exception.php with a namespaced WpOrg\Requests\Exception class.
  • When the database upgrade is called and Requests_Transport_cURL->process_response() results in a cURL timeout, it tries to throw Requests_Exception. Fatal.

The Solution

We can resolve this fatal by including the following line at the top of wp-admin/includes/update-core.php in the upgrade package:

<?php

require_once ABSPATH . 'wp-includes/Requests/Exception.php';

Why?

  • Requests_Exception is used before Requests 2.0.
  • WpOrg\Requests\Exception is used after Requests 2.0.
  • Both use the same file.
  • The exception class for the current Requests library used by the site needs to be loaded into memory before the upgrade process begins.

What else do we need to do?

  • Update every upgrade package on wp.org for WordPress versions that use Requests.
  • This has been confirmed as needed for earlier WordPress versions.
  • Adding this fix to an earlier upgrade package before rolling back has been confirmed as working by myself and @mkaz.
  • This means that we don't have to force a maintenance update on existing installs. When they try to upgrade or rollback, the package will have what they need.

Update on cURL timeout

The Cause

  • This appears to be an issue contacting wordpress.org or with Apache.
  • Props to @pbiron for reporting this timeout during testing:
PHP Warning:  An unexpected error occurred. Something may be wrong with WordPress.org or this server&#8217;s configuration. If you continue to have problems, please try the <a href="https://wordpress.org/support/forums/">support forums</a>. (WordPress could not establish a secure connection to WordPress.org. Please contact your server administrator.)
  • The timeout also occurred on Apache for me.
  • The timeout doesn't happen every time.
  • This doesn't occur on NGINX for me or for @mkaz.

The Solution

  • Technically, if a timeout occurs, it should throw the above warning.
  • @pbiron notes that increasing the timeout for the Requests library has proven useful for avoiding timeouts. However, this timeout should not be happening in the first place. The default timeout is 60 seconds to contact wp.org.
  • Resolving the timeout requires more investigation.

Why?

  • The timeout, at least in some cases, occurs at the "Upgrading Database..." step.
  • I haven't looked into whether or not the database gets upgraded before this fails.
  • The timeout, at least in some cases, causes the updater lock to remain in place, blocking repeat updates without deleting the entry in the database.

What else do we need to do?

  • Investigate the cause of this particular timeout.
  • Determine whether it is only occurring on Windows with Apache.
  • Explore a solution.
  • Test.

Huge props to @pbiron and @mkaz for helping to test this fix.

This ticket was mentioned in PR #1999 on WordPress/wordpress-develop by costdev.


3 years ago
#28

  • Keywords has-patch added; needs-patch removed

SergeyBiryukov commented on PR #1999:


3 years ago
#29

Thanks for the PR! Just to clarify, this would only be needed for 5.8 and older branches, but not for trunk, right?

Should we also add a comment to explain this line?

costdev commented on PR #1999:


3 years ago
#30

Thanks for the PR! Just to clarify, this would only be needed for 5.8 and older branches, but not for trunk, right?

This line will be needed for every branch of WordPress that uses the Requests library (both past and future branches).

Why?

  • Every < 5.9.0 branch can upgrade to > 5.9.0 and will fatal on a missing Requests_Exception class.
  • Every > 5.9.0 branch will be able to rollback to < 5.9.0 and will fatal on a missing WpOrg\Requests\Exception class.

This line will only need to exist in the upgrade packages.

Why?

  • The upgrade package's wp-admin/includes/update-core.php file is the one that runs during the upgrade process.
  • This will provide the fix to all existing installs upon upgrade, negating the need to force a maintenance update on existing installs.

Upgrading to 5.9.0 Beta 1 and rolling back to 5.8.2 has been tested by myself and @mkaz.
This test used a custom upgrade package with the line added to its wp-admin/includes/update-core.php file.

Should we also add a comment to explain this line?

Absolutely. How does this sound?

{{{php
/

  • Namespacing was introduced in Requests 2.0.
  • This file MUST be loaded into memory before the upgrade process
  • to prevent a fatal error due to class names that no longer exist
  • after the upgrade.
  • This applies to all WordPress versions that use the Requests library. */

require_once ABSPATH . 'wp-includes/Requests/Exception.php';
}}}

hellofromtonya commented on PR #1999:


3 years ago
#31

This line will be needed for every branch of WordPress that uses the Requests library (both past and future branches).

@costdev The upgrader uses the "to" version wp-admin/includes/update-core.php file, meaning only 5.9 needs this code. For older WordPress versions upgrading to 5.9, this PR will ensure that missing "from" version exception file is loaded into memory from the older WordPress version.

Why then does this need to be backported to all the older branches?

hellofromtonya commented on PR #1999:


3 years ago
#32

The upgrader uses the "to" version wp-admin/includes/update-core.php file, meaning only 5.9 needs this code. For older WordPress versions upgrading to 5.9, this PR will ensure that missing "from" version exception file is loaded into memory from the older WordPress version.

Why then does this need to be backported to all the older WordPress branches?

Sharing a conversation with @costdev.

The Upgrader does load and use "to" version's wp-admin/includes/update-core.php file. This means upgrading from an older version of WordPress ("from" version) to 5.9 ("to" version) is handled by 5.9.

Back downgrading (rolling back) to an older version from 5.9 will cause a fatal error for the same reason as upgrading to 5.9. So the backports are required to ensure the older ("to") version's wp-admin/includes/update-core.php file also loads the needed file(s) into memory to avoid the fatal error.

Backporting does not require a point release. Why? The Upgrader fetches the "to" version from wp.org. Backporting then upgrades the upgrader package (wow that was a clumpsy way to phrase it) for use when rolling back from WordPress 5.9.

hellofromtonya commented on PR #1999:


3 years ago
#33

Update:
@costdev and I experimented with creating a "preloader" to load all "from" version files into memory (including the entire Requests library). We have a working model. There are pain points to resolve in downgrading from 5.9. Plus a preloader introduces complexity that needs extensive testing to avoid introducing unexpected upgrade issues. Given both of these points, it's too late in this beta period to introduce a "preloader".

Instead, let's first preload the file that directly caused the reported fatal error. Then continue monitoring feedback for other files that _may_ also need preloading.

A "preloader" can be explored during 5.9 if the entire Requests library does need preloading. Else, it can be explored in a future release.

#34 @hellofromTonya
3 years ago

  • Keywords commit added

PR 1999 directly resolves the fatal error by preloading the wp-includes/Requests/Exception.php file from the "to" version of WordPress before the files on the filesystem are upgraded to 5.9.

It's ready for commit.

Note: This PR does not resolve the cURL timeout issue.

#35 follow-up: @hellofromTonya
3 years ago

@costdev @pbiron Should the cURL timeout value be increased to give more time to avoid the timeout error?

#36 in reply to: ↑ 35 @pbiron
3 years ago

Replying to hellofromTonya:

@costdev @pbiron Should the cURL timeout value be increased to give more time to avoid the timeout error?

I'm not sure that's necessary. In most "real world" cases, when timeouts occur it's because there is a problem with the connection (or the remote server) and having an unduly long timeout just delays the inevitable timeout error :-)

The default for the wp_remote_{method}() functions is 5 seconds (filterable by http_request_timeout, see L161, wp-includes/class-wp-http.php), but various different calls to those functions throughout core often pass different timeout values in the $options param (e.g., wp_version_check() uses 3 seconds).

For me, that is too short, but that's just because my home internet connection is soooooo slow (satellite provider): average ping times for me are in the 600-900 ms range :-(. So, on all the sites I run locally, I hook into http_request_timeout and add an extra 10 seconds.

For those with normal connection speeds, I think the defaults currently used throughout core are fine...but certainly wouldn't object if others wanted to increase them.

#37 @pbiron
3 years ago

  • Description modified (diff)

pbiron commented on PR #1999:


3 years ago
#38

@hellofromtonya see https://github.com/WordPress/wordpress-develop/pull/1999#issuecomment-985265243. I think it would be good to add that inline comment (or something like it) before committing.

SergeyBiryukov commented on PR #1999:


3 years ago
#39

The Upgrader does load and use "to" version's wp-admin/includes/update-core.php file. This means upgrading from an older version of WordPress ("from" version) to 5.9 ("to" version) is handled by 5.9.

Back downgrading (rolling back) to an older version from 5.9 will cause a fatal error for the same reason as upgrading to 5.9. So the backports are required to ensure the older ("to") version's wp-admin/includes/update-core.php file also loads the needed file(s) into memory to avoid the fatal error.

Backporting does not require a point release. Why? The Upgrader fetches the "to" version from wp.org. Backporting then upgrades the upgrader package (wow that was a clumpsy way to phrase it) for use when rolling back from WordPress 5.9.

I'm not quite sure about the latest part. Is it about a manual downgrade or an automated rollback if the background update didn't work? Isn't the rollback package only updated once a new minor version is released?

#40 @hellofromTonya
3 years ago

Replying to @jrf:

Would moving the files to wp-includes/requests/src be a potential solution ? (to make sure they are seen as new files, not updated files)

Moving the Requests library into a different directory would solve the issue reported in #54582; however, it would not solve the issue reported in this ticket.

Why? The filesystem is changed and old files removed before `wp_remote_post()` is called to upgrade the database. wp_remote_post() uses WP_Http which uses Requests.

#41 @hellofromTonya
3 years ago

Bigger picture of this problem:

This ticket revealed a fundamental problem in Core's upgrader: the current ("from") version code needs to be preloaded into memory for use before doing the filesystem and database upgrade to the "to" version.

Why? To avoid an intermingling of "from" and "to" version code and/or missing dependencies that can cause errors or expected behaviors.

Though this ticket is highly specific to a single exception file that is loaded when needed, there also lies other unknown issues where "to" and "from" code could be intermingling leading to unexpected (and hard to debug) behavior during the upgrade.

How so? Core code is changed throughout the release cycle. If any of that code is needed but not preloaded before upgrading, then the "to" code will get loaded and may not behave the same as the "from" code version or may expect different dependencies.

Stop Gap fix

PR 1999 is a stop gap fix. It does not resolve the bigger problem in the upgrader. And there's a risk that other files may also need preloading in different edge cases that could also cause a fatal error.

Audit and Preloader

I agree with @jrf. An audit is needed to identify all of the files the upgrader process will or could use (such as alternate code, e.g. exceptions) and then ensure each is preloaded into memory before doing the filesystem upgrade. A preloader though needs extensive testing and likely should be early in a release cycle. Touching the upgrade this late is risky.

I'll open a new ticket for the upgrade audit and preloader.

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


3 years ago

#43 @hellofromTonya
3 years ago

  • Keywords commit removed
  • Milestone changed from 5.9 to 6.0

Decision was made to revert Requests 2.0.0 and punt it and all associated tickets to WordPress 6.0. Why? There are 2 issues in the upgrader process that need resolution, which will resolve the fatal error reported in this ticket.

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


3 years ago

#45 @Grzegorz.Janoszka
3 years ago

Hi, found this bug while googling for string "wp-admin/includes/file.php on line 1201".

I upgraded to 5.9 yesterday but every now and then I receive an error from cron:

PHP Warning: rename(wordpress-5.9-pl_pl.zip): Failed to open stream: Permission denied in /z/wp-admin/includes/file.php on line 1201
PHP Warning: rename(/tmp/pl_PL-4Lxdqs.tmp,wordpress-5.9-pl_pl.zip): Permission denied in /z/wp-admin/includes/file.php on line 1201

All rights to WP directory and /tmp are fine.

#46 @jrf
3 years ago

@Grzegorz.Janoszka As the changes this ticket refer to were reverted, your issue is unrelated. Please open a new, separate issue.

#47 @Grzegorz.Janoszka
3 years ago

Thank you, opened #54946.

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


3 years ago

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


3 years ago

#50 @costdev
3 years ago

  • Milestone changed from 6.0 to Future Release

Moving this ticket to Future Release in line with the Requests 2.0.0 ticket #54504.

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


3 years ago

#52 @costdev
3 years ago

  • Milestone changed from Future Release to 6.1

This ticket was discussed during the recent bug scrub.

As the Requests 2.0 ticket (#54504) has been milestoned for 6.1 and this issue is a blocker for Requests 2.0, I'm moving this ticket to the same milestone.

Note: PR 1999 is a stopgap fix that can be implemented ahead of investigation and implementation of #54589.

While it's possible that there are other Core files required during upgrade which are not preloaded, I believe that we don't have any other reports at this time. A deep-dive to investigate this further is planned as part of #54589.

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


3 years ago

costdev commented on PR #1999:


3 years ago
#54

I've just pushed an update that should cover downgrades without needing to backport the require_once line.

TL;DR - The newer WordPress version applies the fix in both of the necessary locations and the older WordPress version doesn't need to do anything.

  • When upgrading: wp-admin/includes/update-core.php is owned by the "to" version.
  • When downgrading: wp-admin/includes/class-wp-upgrader.php is owned by the "from" version.

Therefore:

  • Upgrading from 5.8.4 to 6.1.0 will use 6.1.0's wp-admin/includes/update-core.php file, which will load 5.8.4's Requests_Exception class into memory before overwriting its contents with 6.1.0's WpOrg\Requests\Exception class.
  • Downgrading from 6.1.0 to 5.8.4 will use 6.1.0's wp-admin/includes/class-wp-upgrader.php file, which will load 6.1.0's WpOrg\Requests\Exception class into memory before overwriting its contents with 5.8.4's Requests_Exception class.
  • The "downgrading" portion is only ever going to be done from a version of WordPress that includes Requests, so it doesn't need a version check.
  • The "upgrading" portion could be upgrading from any earlier version, so it needs to keep the version check already included in wp-admin/includes/update-core.php.

Happy to answer any questions to clarify anything about this update.

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


3 years ago

#56 @costdev
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from 6.1 to Future Release

As the Requests 2.0.0 ticket (#54504) has been moved to Future Release, I'm milestoning this for the same.

To move this ticket forward, I'd appreciate some feedback on this comment on PR 1999.

Note: See TracTickets for help on using tickets.