This is the 8.x-1.x version of the issue with the same name for 7.x. This same deadlock bug applies to both.

Comments

jonathan.green created an issue. See original summary.

jonathan.green’s picture

StatusFileSize
new999 bytes

This is the same issue as in the 7.x branch, the code used to run imagemagick is basically the same. This is a port of the patch I made for the 7.x branch.

Original Comment:

There is a race condition where the stderr pipe can fill up while we are reading the stdout pipe. This causes imagemagic to stop processing and wait for the pipe to empty, since we continue to read the stdout pipe until it closes, we end up in a deadlock with php waiting for the application to close stdout and the application is waiting for php to read the data from stderr.

mondrake’s picture

Version: 8.x-1.x-dev » 8.x-2.x-dev
Status: Needs review » Needs work
Issue tags: +Needs tests, +needs backport to 8.x-1.x

Thank you @jonathan.green

We need to fix this in 8.x-2.x first. The patch does not apply there, unfortunately, as that piece of code was moved to the ImagemagickExecManager class.

Also, it would be great to have tests to demonstrate the failure.

jonathan.green’s picture

StatusFileSize
new955 bytes

Here is the patch updated to 8.x-2.x, the code is again basically the same, just in a different place.

Testing this is tricky. I think it needs more testing-fu then I have. Maybe something like shipping with a bad PDF in the test folder then running a convert on it. Not sure exactly. I'm fairly convinced it will depend on the system installed versions of IM, GS and the libopenjp2, because that is what determines how much output goes to stdout.

jonathan.green’s picture

Status: Needs work » Needs review
mondrake’s picture

Thanks @jonathan.green

If anyone can post here an offending PDF (or other formats like broken JPEG as I saw in the D7 version of this issue), of course assuming that it is OK to put in the git repo as a test file, it may help.

A concern I have is that the warning message is logged/presented on screen and would like to ensure a 64k+ string does not create other issues.

mondrake’s picture

Can anybody check if the latest patch in #2911289: Use Symfony process component instead of proc_open would solve this issue?

mondrake’s picture

Status: Needs review » Closed (outdated)

This issue should be overcome in the latest 8.x-2.x-dev release after the commit of #2911289: Use Symfony process component instead of proc_open. Please report back if not.