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

[azaozz]

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.

[azaozz]

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).

2. 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".

[azaozz]

Related: #39647, #40439.

[azaozz]

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.

[azaozz]

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.

[azaozz]

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 :)

[afercia]

@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]

No error messages on media-new.php

[kirasong]

I like where this is going a lot!

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

[azaozz]

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.

[azaozz]

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.

[azaozz]

In 46075:

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

Props afercia.
See #47872.

[azaozz]

In 46081:

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

See #47872.

[azaozz]

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.

[desrosj]

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

[azaozz]

Replying to desrosj:

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

[kirasong]

Expand error code to include 502.

[azaozz]

Right, lets add HTTP 502 error codes too.

[azaozz]

In 46506:

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

Props mikeschroder, azaozz.
Fixes #47872.

[justinahinon]

Mentioned in ​https://make.wordpress.org/core/2019/10/11/updates-to-image-processing-in-wordpress-5-3

[GregLone]

Hello.

It became really difficult to launch an action after all thumbnails are created.
So far we could use wp_generate_attachment_metadata, wp_update_attachment_metadata, updated_post_meta, or added_post_meta hooks, which was already complicated, but now it became almost impossible. Is it possible to create new hooks for that purpose?

Unrelated, what I believe are typos:

• _legasy_support => _legacy_support?
• // the cliend doesn't know the attachment ID yet. => // the client doesn't know the attachment ID yet.?

Thanks.

[azaozz]

Replying to GregLone:

Right. The wp_generate_attachment_metadata will still fire on all (successful) image uploads that are completed during the original upload request, same as before. This is the most common case. If there was a PHP fatal error while creating image sub-sizes, that filter won't fire (won't be reached), again, same as before.

Was thinking how to fire it, or add a new filter/action, for the cases where it initially fails and retrying to create image sub-sizes succeeds. One possibility is to add an action at the end of both wp_create_image_subsizes() and wp_update_image_subsizes(), something like wp_image_subsizes_created. However it still won't be able to pass the context, i.e. whether this is a new upload or regenerating the sub-sizes of an existing/old attachment, so it won't be very straightforward to use.

Any better ideas? :)

what I believe are typos

Thanks for catching these. One was removed and the other fixed in a follow-up ticket, see #48200.

[GregLone]

Indeed, I don't see any way to pass a context to this hook.
By the way, I'm mixing #40439 and this ticket here, tell me if I'm not at the right place to discuss this.

For me, what differentiates new uploads from existing attachments:

• wp_insert_attachment() is called before, so the hook add_attachment is fired.
• Or, the hook wp_ajax_media-create-image-subsizes is fired.

In both cases we (as in "third party plugins", not WP core) can use the fired hook to store the attachment ID, and use it later in the new hook for comparison. It's a bit tricky, not clean, but it works (at least for add_attachment so far). This is actually ​what I do in Imagify.

Since I use updated_post_meta/added_post_meta to trigger what I want to do (automatic optimization of the images) (so I'm sure the post meta value is up-to-date in the DB), it became more difficult to ​trigger only when all is done, and not for each size.

I see that both wp_create_image_subsizes() and wp_update_image_subsizes() use _wp_make_subsizes(), why not adding this hook at the end of it?

[kirasong]

Replying to GregLone:

Indeed, I don't see any way to pass a context to this hook.
By the way, I'm mixing #40439 and this ticket here, tell me if I'm not at the right place to discuss this.

They're definitely related! This ticket is fine.
<snip>

Since I use updated_post_meta/added_post_meta to trigger what I want to do (automatic optimization of the images) (so I'm sure the post meta value is up-to-date in the DB), it became more difficult to ​trigger only when all is done, and not for each size.

I see that both wp_create_image_subsizes() and wp_update_image_subsizes() use _wp_make_subsizes(), why not adding this hook at the end of it?

Yes! The idea is to start performing operations and saving meta per size, rather than per attachment so that users can use the images that have been created so far right away, and so that if an upload fails, it can be resumed.

Could you please walk me through why in this case it's best to run after all current sizes are done, rather than doing so per size? Want to be sure I understand the use-case.

[GregLone]

Hello :)

Sure, this is our use case: optimize the images automatically once they are uploaded.

We could do it after each thumbnail is created, but this is why we decided not to:
Case 1, optimizing synchronously: each thumbnail is optimized in the same thread.

• Upload will take more time, the user will have to wait more to be able to use the images.
• You're sure to have a timeout.

Case 2, optimizing asynchronously: each thumbnail is optimized in another thread.

• If you have 10 thumbnails (+ full size), you will have 11 optimization requests launched approximately at the same time, and at the beginning, working while other thumbnails are generating. It's enough to freeze the web site.

Currently we optimize the images asynchronously with a queue system after all thumbnails are created:

• No timeouts.
• No freezes, the images are optimized each one after the other.
• No delay, the user can work with the uploaded images right away.

[azaozz]

Replying to GregLone:

Currently we optimize the images asynchronously with a queue system after all thumbnails are created.

Yes, this is by far the better option. And not only in another thread, but at a later time, something like 10 min later perhaps? The reason is that the user may be uploading multiple images at the same time. Then if optimization starts right after the first image was successfully uploaded, it will most likely coincide with the post-processing of the second image and may hit any resource limitations on the server regardless that it's in another thread. Basically optimizing image A's sub-sizes while image B's sub-sizes are being created.

Generally (if I'm imagining how this works correctly) optimized and non-optimized images are interchangeable. The optimization replaces the non-optimized image sub-size file with an optimized file. So it seems best to perform the optimization a bit later, when the server is not under heavy load.

I agree, having a definitive "upload is now done and all sub-sizes are created" hook will be helpful. However it's not very straightforward to pass that "context" to wp_update_image_subsizes(). Looking at the AJAX action is not enough as files may be uploaded through the REST API.

At the same time it would be good to catch all uses of wp_update_image_subsizes() regardless if it is run while re-trying after an error during upload or at a later time. Thinking adding an action at the bottom there would make this work better for optimization of the new sub-sizes, or perhaps other actions that plugins want to perform when more/new sub-sizes are created.

[azaozz]

In 47872.3.diff: do an action after all new image sub-sizes have been created successfully in wp_update_image_subsizes().

[GregLone]

I haven’t tested this new hook yet but it seems perfect. Thanks a lot <3

[kirasong]

@azaozz I really like the location of this hook -- thanks!

I'm +1 on committing this.