Don't let PHP timeout in the middle of an ImageMagick operation

[drzraf]

Depending on configuration Imagick processing may take time.

Multiple problems exist if PHP timeout before the ImageMagick completed:

1. Temporary files aren't cleaned by ImageMagick garbage collection
2. No clear error is provided
3. The cause of such timeout can be hard to pinpoint.

This new helper is expected to be run before heavy image routines and resolves above point 1 by aligning Imagick's timeout on PHP one (assuming it's set).

Note: Imagick resource exhaustion do not catchable issue exceptions (but that would be nice to have since we would now be able to handle them).

Actual incident:

One of our declared image size was 3000px wide. But that post was given a mobile (portait-oriented image of 3000px high and only 1666px wide).

The Resizing-logic triggered WP_Image_Editor_Imagick::thumbnail_image().
But while resizeImage would take (only) ~2 seconds, unsharpMaskImage is much more expensive, especially on such big image and was taking way more time than PHP's timeout.

The thread (and the Imagemagick process) was killed, but doing so brutally, ImageMagick left a 128MB temporary-file inside /tmp every time the same HTTP request was triggered.

Said request coming from the frontend (lazy image-resizing) it quickly filled said partition until it disrupted httpd.

That's why I set this ticket's to "defect".

Media: Don't let PHP timeout in the middle of an ImageMagick operation.

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

[audrasjb]

Hi,

I fixed the coding standards issue in the above PR, but there's still an unit tests issue.

Let's move this ticket to the next milestone as 6.1 RC1 is approaching.

This PR is a refresh of #1019, with some docs improvements.

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

[costdev]

I've submitted ​PR 4044 which refreshes PR 1019 with some docblock improvements.

@audrasjb I don't see any unit tests issue in GitHub Actions. Do you remember what this referred to?

[antpb]

In 55348:

Media: Add setImagickTimeLimit() function to avoid timeout in Imagick operations.

Previously, Imagick operations could silently error by timeout and produce unexpected results. The new setImagickTimeLimit() function will better handle garbage collection in these cases as well as better align Imagick's timeout with PHP timeout, assuming it is set.

Props drzraf, audrasjb, costdev.
Fixes #52569.

[SergeyBiryukov]

Thanks for the commit!

Reopening, as [55348] appears to be accidentally committed to the 6.1 branch instead of trunk.

Is there a reason the new method does not follow the snake_case naming used by the other methods of the same class (also recommended by the ​WordPress coding standards)? Could it be renamed to set_imagick_time_limit()?

[SergeyBiryukov]

Some minor edits in 52569.diff​:

• Rename the method to ::set_imagick_time_limit() to match WPCS recommendations and other methods.
• Use (int) and (float) type casting instead of intval() and floatval(), see [49108].
• Call ::set_imagick_time_limit() a bit later in ::resize() to avoid a double call in case $crop is true.
• Minor documentation adjustments.

[SergeyBiryukov]

In 55403:

Media: Revert [55348] from the 6.1 branch.

This will be committed to trunk instead.

See #52569.

[SergeyBiryukov]

In 55404:

Media: Add WP_Image_Editor_Imagick::set_imagick_time_limit() method.

This aims to avoid timeout in Imagick operations.

Previously, Imagick operations could silently error by timeout and produce unexpected results. The new ::set_imagick_time_limit() method, now used in ::resize() and ::crop(), will better handle garbage collection in these cases as well as better align Imagick's timeout with PHP timeout, assuming it is set.

Props drzraf, audrasjb, costdev, antpb, SergeyBiryukov.
Fixes #52569.

[milana_cap]

Added to misc dev note. Draft: ​https://make.wordpress.org/core/?p=103089&preview=1&_ppp=36765ffd5f