Make WordPress Core

Opened 3 years ago

Closed 14 months ago

Last modified 14 months ago

#52569 closed defect (bug) (fixed)

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

Reported by: drzraf's profile drzraf Owned by: antpb's profile antpb
Milestone: 6.2 Priority: normal
Severity: normal Version: 5.6.1
Component: Media Keywords: has-patch needs-dev-note
Focuses: Cc:

Description

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

Attachments (1)

52569.diff (4.1 KB) - added by SergeyBiryukov 14 months ago.

Download all attachments as: .zip

Change History (18)

This ticket was mentioned in PR #1019 on WordPress/wordpress-develop by drzraf.


3 years ago
#1

  • Keywords has-patch added

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

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

#3 @SergeyBiryukov
22 months ago

  • Milestone changed from Awaiting Review to 6.1

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


21 months ago

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


19 months ago

#6 @antpb
19 months ago

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

#7 @audrasjb
19 months ago

  • Milestone changed from 6.1 to 6.2

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 ticket was mentioned in Slack in #core by costdev. View the logs.


14 months ago

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


14 months ago
#9

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

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

#10 @costdev
14 months ago

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?

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


14 months ago

#12 @antpb
14 months ago

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

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.

#13 @SergeyBiryukov
14 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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()?

#14 @SergeyBiryukov
14 months ago

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.

#15 @SergeyBiryukov
14 months ago

In 55403:

Media: Revert [55348] from the 6.1 branch.

This will be committed to trunk instead.

See #52569.

#16 @SergeyBiryukov
14 months ago

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

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.

#17 @milana_cap
14 months ago

  • Keywords needs-dev-note added
Note: See TracTickets for help on using tickets.