Review: Boost.Threads

From: Peter Dimov (pdimov_at_[hidden])
Date: 2001-08-31 09:59:47

General comments:

The library is too big. Casting a simple accept/reject vote is difficult.

I don't consider phase 2 complete. Adding thread management functionality
may result in significant changes in the underlying implementation and may
even affect the design.

The contents of the library - including some pretty generic names such as
'condition', while residing in boost/thread/*.hpp, are placed directly in
namespace boost. An alternative would be to move them into namespace
boost::thread. This would probably mean renaming the boost::thread class
into something like boost::thread::object. boost::thread::sleep and
boost::thread::yield will move to namespace scope where they belong (they
don't need to access the thread class.)

The reference for many classes indicates that they derive from
boost::noncopyable. This implies, AFAICS, that a C-style cast to
boost::noncopyable* works. If retained, all such derivations need an
'exposition-only' comment.

The documentation uses boost::xtime and several other related identifiers
without defining them anywhere.

Index:

The 'tss' link is broken.

Introduction:

The 'thread-safe' and 'tss' links are broken.

Semaphore:

The rationale didn't convince me of the need for having such a dangerous
primitive. It would help if "other synchronization primitives" is expanded
to mention concrete examples.

In semaphore::up() 'ct' is not used.

The semaphore example uses boost::thread::create which is either obsolete or
a future extension. ;-)

Mutex Concept:

M m(); is a function declaration, so the Effects clause is unimplementable.
It should probably be M m;

Mutex:

The reference section refers to the introduction for explanation of the
scoped_lock nested type. I think that the reference should be
self-contained.

The example uses boost::thread::create and boost::thread::join_all.

The mutex is missing one bit of functionality that I consider critical. When
two threads need to lock two (or more) mutexes, the lock order must be the
same to avoid deadlocks. No support is provided for this operation. Possible
alternatives are

template<class M1, class M2, ...> class scoped_multiple_lock
{
    scoped_multiple_lock(M1 &, M2 &, ...);
};

or

template<class M1, class M2, ...> class mutex_tuple
{
    typedef ... scoped_lock;
    mutex_tuple(M1 &, M2 &, ...);
};

An additional point that will become relevant when thread cancelation is
implemented. Is locking a mutex a cancelation point? I think that, by
default, it should be, since it blocks; in which case we need another way to
lock that would not be a cancelation point (allowing its use in
destructors.)

Lock Concepts:

Broken link: 'thread-safe'.

I believe that "operator const void * () const" shouldbe removed. locked()
is enough.

The usage idiom for ScopedTryLock and ScopedTimedLock constructors that call
try_lock and timed_lock, respectively, is not clear to me. It seems that

Lock l(m);
if(l.locked()) { ... }

is exactly the same as

Lock l(m, false);
if(l.try_lock()) { ... }

except that the latter is more explicit. Having a constructor like "Lock
l(m)" with the same syntax and subtly different semantics (for ScopedLock
and ScopedTryLock) is a potential source of errors.

Scoped_lock:

scoped_lock is defined in namespace boost by the reference, in namespace
boost::detail::thread in the implementation, and in namespace
[boost::]detail by the mutex documentation.

scoped_lock is in header boost/thread/xlock.hpp... it should be in
boost/thread/detail/lock.hpp in my opinion.

Scoped_try_lock, scoped_timed_lock: see above.

Condition:

"The mutex model must be locked..." Can a model be locked?

I think that adopting stabdard POSIX terminology - signal/broadcast instead
of notify_one/notify_all - deserves consideration.

wait(lock, pred):

"while !(pred())" should be "while(!pred())"

I'd like to see

template<class M, class P, class F> void wait(M & m, P p, F f)
{
    typename M::scoped_lock lock(m);
    while(!p()) this->wait(lock);
    f();
}

added, for people that want to go the extra mile (when someone bothers to
define a predicate to use the wait(l, p) form, (s)he would probably define
an action object, too.)

The example uses boost::thread::create and boost::thread::join_all.

Thread_specific_ptr:

"... and deletes the object when the thread terminates." should probably be
" ... and calls delete on the contained pointer when the thread terminates."

In reset(): " The pointer will be deleted when the thread terminates."

The example uses the obsolete tss class.

Thread:

In the default constructor: 'Danger: *this is valid only within the current
thread.' This requirement should probably be removed. If it's not, it should
be moved to Effects.

thread(const function0<void> &) should be explicit:

boost::function0<void> f;
boost::thread t;

t == f;

The effects clause should mention that the constructor constructs a thread
object.

operator==, !=:

"*this and rhs represent the same thread of execution." The constructors do
not mention that the constructed thread objects represent a thread of
execution, and if so, which one.

join:

I prefer the 'no effect on second join, serialized' semantics, as outlined
in my other posts.

The precondition '*this != thread()' is a specific instance of a larger
problem. I don't really have a problem with it -- except that it turns a
well-defined behavior (deadlock) into undefined behavior (is this worth
it?), but I prefer something along the lines of 'The implementation is
allowed but not required to detect a deadlock when this method is called.
When the implementation does detect a deadlock, the behavior is
implementation defined.'

sleep:

'... until xt.' xtime is not defined, but even if it was, the terminology
'until xt' is not clear. 'until xt is reached', perhaps?

Thread_group:

In general, I'm not enthusiastic about the thread_group class. It's neither
a low-level primitive nor a high-level safe container. I'd like to see it
removed from this initial submission - and re-introduced later, if
necessary, although I'd rather see a thread::ref high level design.

add_thread: the function takes ownership of the passed pointer. The usual
C++ idiom is to use an std::auto_ptr argument to make the ownership transfer
explicit. This will also eliminate the bug in the implementation where the
passed pointer leaks when m_threads.push_back throws.

remove_thread: the function transfers ownership of the pointer to the user.
The usual idiom is to return an std::auto_ptr.

join_all: the implementation will deadlock when one of the contained threads
waits on m_mutex. It should execute the join operation on a copy of the
list, outside of the lock.

Lock_error:

"thrown by lock operations that would otherwise deadlock" - I don't like the
wording. It seems to imply that the implementation would detect deadlocks
and throw lock_error on occasions that are not documented in the 'Throws'
clause of the corresponding lock operation.

FAQ:

#4: the assignment operator is deadlock prone due to the unordered double
lock.

Once:

once.hpp defines boost:: names that are not documented anywhere. They are
useful primitives and need documentation.

Overall vote: accept; although I expect the 'thread' class to evolve
significantly as more functionality is added.

--
Peter Dimov
Multi Media Ltd.