WordPress.org

Make WordPress Core

Opened 2 months ago

Closed 3 days ago

Last modified 3 days ago

#47872 closed enhancement (fixed)

Try to create image sub-sizes again when uploading fails with a PHP fatal error

Reported by: azaozz Owned by: azaozz
Milestone: 5.3 Priority: high
Severity: normal Version:
Component: Upload Keywords: has-patch needs-testing has-screenshots close has-dev-note
Focuses: Cc:
PR Number:

Description

A common reason uploading of images fails is because the server runs out of resources while creating image sub-sizes. That results in either a timeout or out of memory errors.

In these cases the image file has been uploaded successfully but some or all of the sub-sizes haven't been created. Because of the PHP fatal error, the response to the browser/client is an "Internal Server Error" (HTTP 500) which currently results in showing an unfriendly/non-actionable "HTTP Error" message in the UI. It also leaves the "orphaned" image sub-sizes that were created before the server crash in the upload directory.

After #40439 and [45538] it became possible to create the missing image sub-sizes when an image file was uploaded successfully. Ideally that should happen right after the file was uploaded, so the user has access to all expected sub-sizes.

Attachments (4)

47872.diff (45.3 KB) - added by azaozz 2 months ago.
Screenshot (22).png (186.5 KB) - added by afercia 6 weeks ago.
Screenshot (21).png (175.1 KB) - added by afercia 6 weeks ago.
No error messages on media-new.php
47872.2.diff (1.7 KB) - added by mikeschroder 3 days ago.
Expand error code to include 502.

Download all attachments as: .zip

Change History (32)

@azaozz
2 months ago

#1 @azaozz
2 months ago

  • Keywords has-patch needs-testing needs-dev-note added

In 47872.diff:

  • When an image upload fails with HTTP 500 error do another AJAX request to try to create any missing image sub-sizes. This currently works in the Media modal and on the Media Library screen, including support for the old inline uploader in Media => Add New. This is attempted three more times.
  • Uses a temporary uploaded file reference (for images only) to retrieve the new attachment_id (saved in a transient for 6 hours).

Currently there are no specific UI changes but the HTTP 500 errors are visible in the browser console. If it still fails after four attempts (the initial upload plus three re-tries) it shows the error messages that come from the server, if any. Then it falls back to the default HTTP 500 error (the text was updated to better reflect what's going on).

Testing is very much appreciated, although it's not very straightforward. To test best would be to artificially "crash" PHP.

  1. Set max_execution_time=4 in php.ini to cause a timeout. Upload a large image, something like 5 - 6MB and watch the request in the browser tools "network" tab. If it doesn't time out, try larger image or set the max_execution_time to 3 seconds (and don't forget to put it back to the original value when done, default max_execution_time is 30 seconds).
  1. Set memory_limit=64M in php.ini and add define( 'WP_MAX_MEMORY_LIMIT', '64M' ); in wp-config.php to limit the memory. Again, try uploading a bigger image and watch the browser tools for HTTP 500 responses. Couple of tests are possible here. If the memory is low and the image big, it may run out of memory on the initial loading of the image. Then there will be 4 HTTP 500 errors pretty fast one after the other. In that case if you bump the memory limit up by a few MB it will start failing while creating the sub-sizes. Then the result will be several requests the last of which should be a "success".
Last edited 2 months ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


2 months ago

#3 @azaozz
2 months ago

Related: #39647, #40439.

This ticket was mentioned in Slack in #core-restapi by azaozz. View the logs.


2 months ago

#5 @azaozz
2 months ago

There is an alternative way I've been testing, noting it here for completeness. Instead of sending an "upload reference" with the upload request, it could try to respond with the new attachment_id right after the attachment was created and "flush" the PHP output (using fastcgi_finish_request()), and continue making image sub-sizes in the background.

That would provide a bit better "responsiveness" in the UI but comes with some strings attached: no sub-size will be immediately available. This method would need quite a lot more "processing logic" in the client. For example if the user just wants to insert an image into a post the client would have to use the original image at first, then request a particular size, and if it was created, replace the image.

If the required sub-size is not yet created, the client will have to try again later. That process may take quite long on slow/busy servers, 30 seconds or more. In the background if PHP fails to make all sub-sizes, we can re-try on the subsequent requests from the client (as we know the attachment_id). A pretty complex case would be when the users don't wait that long and close the browser or navigate away. Then all the "replace" logic should happen after the fact on the server, perhaps with a cron job, etc.

From a purely UI/UX perspective thinking that waiting until all sub-sizes are created (the current behaviour) makes more sense. Then all sub-sizes will be available and the users can pick the one they need. When uploading a file, it takes some time and the users expect that. Largely that time will be for the actual upload to go through, not for creating the sub-sizes.

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


8 weeks ago

#7 @joemcgill
8 weeks ago

  • Priority changed from normal to high

This ticket was mentioned in Slack in #core-media by azaozz. View the logs.


7 weeks ago

#9 @azaozz
6 weeks ago

In 45934:

Uploads: After an image is uploaded and PHP times out or runs out of memory during post-processing (the server response is HTTP 500 error), try to resize it three more times. Then, if all attempts fail, do a cleanup of any sub-sizes that may have been created and show an error message asking the user to scale the image and upload it again.

See #47872.

#10 @azaozz
6 weeks ago

Forgot to mention, the patch includes some unrelated WPCS fixes, a lot for the code for the (super old) media iframe popup.

Leaving open as this needs more testing. Seems to work properly here, but more eyes would be great! :)

Also, the general HTTP 500 error message was changed to:

Unexpected response from the server. The file may have been uploaded successfully. Check in the Media Library or reload the page.

And the HTTP 500 message after uploading an image and post-processing fails after 4 retries was changed to:

Post-processing of the image failed. If this is a photo or a large image, please scale it down to 2500px and upload it again.

Suggestions welcome if these don't sound good :)

#11 follow-up: @afercia
6 weeks ago

  • Keywords has-screenshots added

@azaozz this one was a nice one to test :)

Tested on Windows, where it's easier to alter the PHP parameters (VVV uses Nginx). In my case, for the first test I had to set max_execution_time to 1 to make it timeout. Also, to upload images larger than 2 MB, set both upload_max_filesize and post_max_size to 20M (WP takes the smaller of these two values).

Both tests worked for me, see the requests in the screenshots below. However, on media-new.php I didn't get any error message (see second screenshot).

Quick questions / considerations:

  • Any objections to expand 2500px to 2500 pixels? Because of the way px is announced by screen readers, see for example https://core.trac.wordpress.org/ticket/38932#comment:1
  • all the error <div>s would need a role="alert": I guess that can be added in one of the many pending tickets to improve the media accessibility
  • I see many links href="#" and href='#' that actually behave like buttons. They should be buttons. However, I think most of them are used in the old upload modal. Correct?

(note: this plugin can help in testing the Old Upload Method)

@afercia
6 weeks ago

No error messages on media-new.php

#12 follow-up: @mikeschroder
6 weeks ago

I like where this is going a lot!

Do you see the error handling as a replacement for #39647, or in addition to it?

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 weeks ago

#14 in reply to: ↑ 11 @azaozz
6 weeks ago

Replying to afercia:

Thanks for testing! :)

had to set max_execution_time to 1 to make it timeout. Also, to upload images larger than 2 MB, set both upload_max_filesize and post_max_size to 20M (WP takes the smaller of these two values).

Right, both upload_max_filesize and post_max_size have to be bumped up to be able to upload larger files. Instead of setting max_execution_time to 1, could you try setting it to 3-4 seconds and uploading a larger image file? A "standard" cellphone photo will probably do. There seem to be in the 4-5MB range.

Both tests worked for me, see the requests in the screenshots below. However, on media-new.php I didn't get any error message (see second screenshot).

Think this is due to setting max_execution_time = 1. On the last request it tries to do some cleanup of possibly left-over image sub-sizes, etc. and 1 second is just too short for "general" wp-admin tasks.

Any objections to expand 2500px to 2500 pixels?

Sure. The text there can be improved. Hoping to get even more eyes on it :)

all the error <div>s would need a role="alert": I guess that can be added in one of the many pending tickets to improve the media accessibility

Haven't changed anything there in the last patch. You're right, better to fix it "globally" for all of the errors.

I see many links href="#" and href='#' that actually behave like buttons. They should be buttons. However, I think most of them are used in the old upload modal. Correct?

Yeah, as far as I see all of these are the old, old upload UI. Not sure if it is used these days... Thinking this should be part of a "refresh" of the "uploader" UI and code. There's probably no need to use Plupload (as far as I see), its main purpose was to enable multi-file uploading and progress bars in old browsers. All new browsers support that natively.

#15 in reply to: ↑ 12 @azaozz
6 weeks ago

Replying to mikeschroder:

Do you see the error handling as a replacement for #39647, or in addition to it?

Thinking as an addition? It would be nice to show the exact cause of the server error, but by itself that doesn't give any "clues" to the user what to do next. The changes here are mostly to add these clues.

#16 @azaozz
6 weeks ago

In 46075:

Uploads: Improve the error message shown when all attempts to create image sub-sizes fail.

Props afercia.
See #47872.

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


6 weeks ago

#18 @azaozz
6 weeks ago

In 46081:

Upload: Reset the temp upload reference in Plupload when the file is not an image.

See #47872.

This ticket was mentioned in Slack in #core-editor by azaozz. View the logs.


6 weeks ago

#20 @azaozz
4 weeks ago

In 46174:

Uploads: add helper functions for setting, getting, and deleting the temp upload reference used to the attachment_id when retrying to make image sub-sizes.

See #47872.

This ticket was mentioned in Slack in #core-media by mike. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core-media by ramonfincken. View the logs.


4 weeks ago

#23 follow-up: @desrosj
4 weeks ago

  • Keywords close added

@azaozz Is there any work left here? Or just a needed dev note?

#24 in reply to: ↑ 23 @azaozz
4 weeks ago

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

Replying to desrosj:

Just the dev note, and perhaps some tests. Any bugs/issues should go in new tickets, perhaps better to close this :)

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


2 weeks ago

@mikeschroder
3 days ago

Expand error code to include 502.

#26 @azaozz
3 days ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Right, lets add HTTP 502 error codes too.

#27 @azaozz
3 days ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from reopened to closed

In 46506:

Uploads: Retry to post-process images after HTTP 500 and HTTP 502 errors.

Props mikeschroder, azaozz.
Fixes #47872.

Note: See TracTickets for help on using tickets.