#63 Wait until all OpenMP threads are closed in DestroyMagick()

[Arjen de Korte]

Currently, there are build failures with php7-gmagick in PHP 7.4 when multi threading is enabled (the default). See https://bugs.php.net/bug.php?id=78465 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91256

The extension calls DestroyMagick() just before closing. If running in single thread, all is well, but if running in multi thread the extension segfaults. Adding a 10 ms delay between calling DestroyMagick() and closing the extension works around this, so this appears to be a race condition - the threads opened by OpenMP don't close quickly enough and are still active when DestroyMagick() returns.

Since OpenMP 5 one can call omp_pause_resource_all(omp_pause_hard) that will relinquish resources used by OpenMP. Doing so right before returning from DestroyMagick() solves the problem also and seems to be a more robust way, since it doesn't require waiting an arbitrary amount of time to allow threads to close and also allows running in multi thread mode.

The attached patch will call this function if it is available right before returning from DestroyMagick().

[Bob Friesenhahn]

On Fri, 3 Jan 2020, Arjen de Korte wrote:

The extension calls DestroyMagick() just before closing. If running in single thread, all is well, but if running in multi thread the extension segfaults. Adding a 10 ms delay between calling DestroyMagick() and closing the extension works around this, so this appears to be a race condition - the threads opened by OpenMP don't close quickly enough and are still active when DestroyMagick() returns.

Since OpenMP 5 one can call omp_pause_resource_all(omp_pause_hard) that will relinquish resources used by OpenMP. Doing so right before returning from DestroyMagick() solves the problem also and seems to be a more robust way, since it doesn't require waiting an arbitrary amount of time to allow threads to close and also allows running in multi thread mode.

The attached patch will call this function if it is available right before returning from DestroyMagick().

If the software using GraphicsMagick also uses OpenMP, it seems like
this approach might cause harm. Worker threads are implicitly created
by the OpenMP implementation. If no images are currently being
handled, then there should be no OpenMP threads active in
GraphicsMagick code.

It seems necessary to get to the bottom of the underlying problem and
determine how OpenMP threads could still be using some
structures/objects created by GraphicsMagick when DestroyMagick() is
called.

Is it possible that PHP 7.4 uses garbage collection, static C++ object
destruction, or some other implicit caching which might cause the user
to think that resources have been released but they are actually still
active?

Bob

[Bob Friesenhahn]

Is php7-gmagick built as a loadable module (a shared library loaded
via dlopen())?

I have encountered issues before with some Linux versions where it
seemed that OpenMP did not play well with loadable module
initialization/destruction. Due to this, some GraphicsMagick coder
modules which might benefit from threading do not use OpenMP by
default.

Loadable modules are actually quite tricky since OpenMP (GOMP, in this
case) might have already been spun up when the module is loaded, or it
might not have been spun up until the module was loaded. A loadable
module is really a shared library which has implicit (and possibly
explicit) constructor/destructor functions just like any other Linux
shared library. Unloading the module might be destroying something
which OpenMP is still using, or it may be that PHP has retained
references to objects/data which were destroyed by unloading the
module.

If php7-gmagick is a loadable module then it likely links with
libgomp, but it is likely that PHP itself does not link with libgomp
if it does not otherwise use OpenMP.

Bob

[Arjen de Korte]

The php7-gmagick extension is indeed a loadable module, PHP doesn't use OpenMP. The code in question is the following:

PHP_MSHUTDOWN_FUNCTION(gmagick)
{
    DestroyMagick();
    return SUCCESS;
}

This is called upon module unload and will segfault under PHP 7.4 when the number of threads is not limited to 1. Putting a usleep(10000) after DestroyMagick() will fix the segfaults, putting it before (no matter how long) makes no difference at all. So I don't think it is PHP referencing objects or data that were destroyed by OpenMP (in that case, the delay wouldn't make a difference).

If we can't call omp_pause_resource_all(omp_pause_hard) in the library (I have to admit, that I didn't consider the possibility that this library might be called by a caller that uses OpenMP too), it might be useful to have a function in the library that would explicitly release whatever OpenMP is using and wait until it is released. So basically a wrapper around this function.

I'm not sure about the garbage collection in PHP. It seems that there have been changes in this area, as in PHP 7.3 this was not a problem (yet).

[Bob Friesenhahn]

On Fri, 3 Jan 2020, Arjen de Korte wrote:

If we can't call omp_pause_resource_all(omp_pause_hard) in the library (I have to admit, that I didn't consider the possibility that this library might be called by a caller that uses OpenMP too), it might be useful to have a function in the library that would explicitly release whatever OpenMP is using and wait until it is released. So basically a wrapper around this function.

I'm not sure about the garbage collection in PHP. It seems that there have been changes in this area, as in PHP 7.3 this was not a problem (yet).

I assume that GCC 10 (which is not released yet) is being used. This
function was not even submitted to GCC until July, 2018 so it is very
new.

Does PHP 7.4 work ok with php7-gmagick on earlier GCC versions or
does it suffer from the same problem? If so, is PHP 7.4 +
php7-gmagick only anticipated to work with GCC 10 and later?

Even if GraphicsMagick was to add this (which I don't think it should
do), it seems short-sighted to require a GCC which is not even
released yet (https://gcc.gnu.org/ml/gcc/2019-11/msg00117.html). It
will be quite a few years before many distributions are offering GCC
10 yet there may be a desire to use PHP 7.4.

Have you addressed this issue with the GCC/GOMP developers? It may be
a bug that they can fix prior to the first GCC 10 release..

Bob

[Arjen de Korte]

This function was released in GCC 9. But the problem it adresses is not GCC version related. Both PHP 7.3 and 7.4 were build with the same GCC version, where the first didn't exhibit this problem with php7-gmagick and the latter did.

It is a known problem with OpenMP: applications have no control over the creation/destruction of threads. See https://github.com/Imagick/imagick/issues/295 for ImageMagick, which suffers from the same problem with PHP 7.4 and OpenMP.

Other than forcing the libraries to run single threaded or adding delays after the DestroyMagick() calls (or MagickWandTerminus() in ImageMagick) and hope for the best there doesn't seem to be a real solution to this problem, other than calling omp_pause_resource_all(omp_pause_hard) when one is sure OpenMP can be terminated. But you're probably right, this should only be done when the caller knows it is safe to do so. Therefor a wrapper in the GraphicsMagick library might be an option.

Note that the attached patch will only call this function when it is available, as it uses autoconf to detect its presence. It is indeed not an all-purpose solution as it will probably take several years until it is omnipresent.

[Bob Friesenhahn]

On Fri, 3 Jan 2020, Arjen de Korte wrote:

Currently, there are build failures with php7-gmagick in PHP 7.4 when multi threading is enabled (the default). See https://bugs.php.net/bug.php?id=78465 and https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91256

The extension calls DestroyMagick() just before closing. If
running in single thread, all is well, but if running in multi
thread the extension segfaults. Adding a 10 ms delay between calling
DestroyMagick() and closing the extension works around this, so
this appears to be a race condition - the threads opened by OpenMP
don't close quickly enough and are still active when
DestroyMagick() returns.

Since OpenMP 5 one can call omp_pause_resource_all(omp_pause_hard)
that will relinquish resources used by OpenMP. Doing so right before
returning from DestroyMagick() solves the problem also and seems
to be a more robust way, since it doesn't require waiting an
arbitrary amount of time to allow threads to close and also allows
running in multi thread mode.

I am concerned that calling omp_pause_resource_all(omp_pause_hard) at
the end of DestroyMagick() may be producing the requisite 10 ms delay
which is needed to avoid the crash. If so, then it may not be curing
anything since it is just causing a delay. This may be what is often
called a "red herring".

Except for the unusual build configuration where the GraphicsMagick
library uses only OpenMP primitives and does not use pthreads (so
OpenMP locks are used as semaphores), GraphicsMagick does not allocate
any objects from OpenMP. Given that any using threads should be idle
before invoking DestroyMagick(), GraphicsMagick should not be using
anything from OpenMP and will not be using any OpenMP worker threads.

Bob

[Bob Friesenhahn]

Interestingly, the bug report at
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1037042 finds that omp_pause_resource_all(omp_pause_soft) helps avoid a memory consumption issue if OpenMP code is invoked by POSIX threads. I mention it since this report uses omp_pause_resource_all(omp_pause_hard) to solve a somewhat similar issue.

[Bob Friesenhahn]

I am wondering if the PHP-multi-threading issue is solved by assuring that no PHP threads are still using the GraphicsMagick APIs before invoking DestroyMagick(). Or alternatively, creating a critical-section/semaphored region which invokes DestroyMagick(), but also calls omp_pause_resource_all(omp_pause_hard).

There have not been any other complaints about such crashes, but if the PHP implementation were to invoke DestroyMagick() while another PHP thread is using GraphicsMagick functions, it would likely case a crash.

If PHP is using loadable modules, then invoking DestroyMagick() while the module is unloaded makes the most sense.