thread lib review

From: John Maddock (John_Maddock_at_[hidden])
Date: 2001-09-07 06:50:50

First of all, let me say that I like the thread lib, and I think that it
should be accepted into boost.  I think that the docs are excellent; more
specific comments follow (based on threadcorrect1.zip) :

Semaphores:
~~~~~~~~~

I think that the semaphore type should be in the lib, However I have one
query about the interface - you have included in the constructor an option
to set the maximum value of the semaphore - as far as I know this is a
Win32 specific feature.  Neither POSIX nor BeOS semaphores support this. 
While it is true that you can emulate this behaviour (probably using two
native semaphores), generally speaking a semaphore should be the most
lightweight synchronisation primitive available, adding this feature makes
it impossible to implement boost::semaphore in terms of a single
lightweight native primitive.

In Semaphore::up the assert:

    assert(ret || GetLastError() == ERROR_TOO_MANY_POSTS);

is failing for the semaphore test in thread_test.cpp, it seems as though
GetLastError is returning ERROR_INVALID_PARAMETER rather than
ERROR_TOO_MANY_POSTS on my Win98 box.

Mutex:
~~~~

Ideally I would like each mutex type to be as efficient as possible: on
win32 that means using critical sections and not mutexes where possible
(mutex and recursive_mutex).  Last I heard, a win32 mutex is about 4x as
slow as a critical section, and given that the simpler primitives here are
likely to be used in "contentious" situations, I would like them to be as
efficient as possible.

Should I be concerned that you cast a HMUTEX to an unsigned long?  Is this
portable to all windows versions (32 and 64 bits, WinCE etc?).  I think I
would be inclined to use a void* throughout, and remove the casts - that
way you'll get an error if its wrong, alternatively add a:
BOOST_STATIC_ASSERT(sizeof(unsigned int) >= sizeof(HANDLE))
to one of the cpp files.

The Win32 mutex constructors look wrong: you have both an assert and a
(currently redundant) throw.

Condition:
~~~~~~
Is it correct that you can use a condition variable with any mutex type? 
I'm concerned that this places an unnecessary restriction on how the mutex
types are implemented, and in particular that it may prevent use of the
most efficient implementation for the simpler mutex types. I'm mainly
concerned about porting to new platforms/api's here.

 What happens if the mutex is a recursive mutex, and has been locked
multiple times - does it deadlock?  If so that would be a good reason not
to use recursive mutexes with condition variables?  

If the condition variable supports timed waits, then does the mutex also
have be timed waitable? (I guess not?)

Once.cpp:
~~~~~~~

You seem to be using swprintf without including the necessary header
(wchar.h), you are also using a non-standard (MS specific) version of that
function, here's what C99 says about it:

7.24.2.3 The swprintf function
Synopsis
1 #include <wchar.h>
int swprintf(wchar_t * restrict s,
size_t n,
const wchar_t * restrict format, ...);

Note the extra size_t parameter - we have a macro for this BTW (or will
have when the new config gets merged in): BOOST_NO_STD_SWPRINTF.

You also use the wide character version of CreateMutex here: but that isn't
supported on Win9x (and fails).

xtime.cpp:
~~~~~~

I would prefer the section:

#else
        return 0;
#endif

to be replaced by

#else
#  error
#endif

It makes it easier to catch a mis-configured set of headers.

Headers:
~~~~~~

Can we have a section that produces a #error if BOOST_HAS_THREADS is
defined, but no supported threading API is configured?

Config:
~~~~

We need some test code for each of the macros (to slot into the
split-config's regression tests, and configure script).
We also need to figure out what the appropriate POSIX feature tests are for
some of these API's, and add them to the new config.

- John Maddock
http://ourworld.compuserve.com/homepages/john_maddock/