--disable-thread confusion

[rrt_wsi]

I've been reviewing my companies distribution build of magick and found some confusing behavior in the thread settings.

The documentation indicates that the default build is thread safe.
I build the default build.
I break into the code using the totalview debugger and examine magick_info.thread_support. The value is MagickTrue (1).

At this point everything makes sense and it appears that we have a thread safe build. Until I look at the following code:

if (GetMagickThreadSupport(magick_info) == MagickFalse)
AcquireSemaphoreInfo(&constitute_semaphore);
status=GetMagickEncoder(magick_info)(write_info,image);
if (GetMagickThreadSupport(magick_info) == MagickFalse)
RelinquishSemaphoreInfo(constitute_semaphore);

This appears to read "If we do NOT have thread support lock and unlock the mutexes" which seems backward to me.

Must we set thread support false in order to have a thread safe library?

Bob

[magick]

ImageMagick is thread safe but some coder modules are not (e.g. JPEG). If a coder modules is not thread safe we serialize access to the module with our own mutexes. More recent versions of ImageMagick distinguish coder module thread support for reading and writing (some modules are threaded when reading an image but not for writing or vise versa).

[rrt_wsi]

Thank you for restating the documentation. Could you please actually read the content of my post?

In short if thread_support == true then no mutexes are used. How is this thread safe?

[magick]

That simply means you are using a coder module that is thread safe so no mutexes are required. The module can run in multiple threads of execution without any problem.

[rrt_wsi]

Ok - then

1 - Why does constitute_semaphore exist?
2 - Why on earth would you use it if disable-thread is true and not use it if disable-thread is false?

Specifically I am looking at constitute.c lines 388 - 1003

Someone at some time thought there was a potential race condition here and added the mutex to make this area of code thread safe. Even if the coder is thread safe it's access may not be as well as any auxiliary buffers etc. Our products have been showing instabilities in ImageMagick when used in a mulithreaded mode. Otherwise I would not be studying this.

Bob

[magick]

1 - Why does constitute_semaphore exist?

To serialize any coder that is declared not thread safe.

2 - Why on earth would you use it if disable-thread is true and not use it if disable-thread is false?

We use it if threading is enabled or not. If threading is not enabled its simply a no-op.

Someone at some time thought there was a potential race condition here and added the mutex to make this area of code thread safe.

The ImageMagick developers did not design the code due to a potential race condition. It was written specifically to permit non-thread safe coders to coexist with the thread-safe ImageMagick API.

We should mention that we do have a regression test to test ImageMagick in multiple threads of execution. ImageMagick is not released unless this test passes. However, we recognize that just because the regression test passes it does not mean there is not something lurking in the ImageMagick API that is not thread safe. If you identify something, let us know.