RE: Review: Boost.Threads

From: William Kempf (williamkempf_at_[hidden])
Date: 2001-08-31 13:55:03

From: "Peter Dimov" <pdimov_at_[hidden]>
>General comments:
>
>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.

Effecting the implementation should be irrelevant, no?  I don't expect the 
interface will be changed because of the addition of thread management 
functionaltiy (in fact, it should not).

>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.)

I'm willing to move them into a new namespace, but during implementation I 
was basically directed not to.  If we come to consensus on this I'll be 
happy to make any changes.

>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.

They indicate private derivation, so the cast isn't valid.  So I'm not sure 
the comment is needed, but I'm happy to add it.

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

Oops!  Big mistake on my part!  I knew this had to be documented before 
submission, but my life has been so hectic the last two weeks that I guess 
this slipped my mind.  Poor excuse, but it's all I've got. :(  I'll add 
documentation for this over the weekend and upload it.  I really appologize 
for this one.

>Index:
>
>The 'tss' link is broken.

Another mistake. "tss" has been replaced by "thread_specific_ptr".  I'll 
correct this.

>Introduction:
>
>The 'thread-safe' and 'tss' links are broken.

I'll correct this as well.

>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.

I'll expand on it.  There are two areas where the semaphore provides utility 
not available from mutexes: ability to be locked in one thread and unlocked 
in another, and the ability to "count" the accesses (in other words, 
restrict access to specific numbers of threads).

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

I'll fix this.

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

*laughs*  Obsolete.  I'll correct this one as well.

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

I'll correct this as well.

>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.

Sorry, I don't follow what you mean here.

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

More stuff that just at the last minute.  I'll correct this as well.

>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 &, ...);
>};

There are numerous methods that can be employed to prevent deadlock.  I 
think this issue is really beyond the scope of the reference documentation 
for mutex.  I can provide a FAQ on this one, which seems more appropriate.  
As for the other concepts... they are worthy of consideration in future 
additions to the library, but aren't really appropriate for release 1.

>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.)

Off topic for the current submission, but...

Yes, locking would be a cancellation point.  However, two versions of 
locking are not needed.  Cancellation concepts must include the ability to 
set the "cancellability state" of the thread, which allows you to lock a 
mutex with out the fear of the thread being cancelled.

>Lock Concepts:
>
>Broken link: 'thread-safe'.

I'll fix this.

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

This was debated and several people prefer the simpler construct of:

if (lock)
  ...

Personally, I don't much care which way we go, but generally it seems as if 
the void* conversion is preferred, at least as an alternate form.  For 
example, the iostreams include this form.  *shrug*  I'll let the review 
process decide this one.

>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.

Hmmm... the interface is intuitive to me, so I never gave this much thought. 
  Seems that users would expect the following to be a try_lock and not a 
lock:

boost::mutex::scoped_try_lock lock(m);

>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.

I'll correct this.  This is something that changed recently.

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

I don't have any strong feelings on this one.  What ever's decided I'll 
follow for this.

>Condition:
>
>"The mutex model must be locked..." Can a model be locked?

*shrugs*  I had help on this one.  The terminology of "concept" and "model" 
were foreign to me when this project was started.  My understanding is that 
the "model" is a concrete representation of the "concept", and as such a 
model of the mutex concept certainly can be locked.

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

It did.  In fact, it generated some debate on this list.  I was for 
signal/broadcast in the beginning, but others convinced me the terms were to 
ambiguous for those not "in the know".  I don't care what names we use here, 
frankly.  I'm tired of name debates :).  What ever is decided in this review 
I'll go with.

>wait(lock, pred):
>
>"while !(pred())" should be "while(!pred())"

I'll correct this.

>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.)

Hmm... never thought about this one.  I don't think I'd ever use this form, 
but I have no problems with including it in addition to the other forms.

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

*sigh*  Really wish I hadn't made the last minute changes with everything 
else that happened during that time period :(.  I'll fix this.

>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."

Sure.  Much better wording.  Thanks.

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

OK.

>The example uses the obsolete tss class.

*blushes*  Will fix.

>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.

OK, this is an optimization detail.  I don't have strong feelings here, 
though others have voiced opinions that they don't like the overhead needed 
in Win32 for a call to DuplicateHandle.  From our other conversations I'm 
convinced there should be a way to get a "current" thread object that's not 
restricted to the current thread.  Would a compromise with the addition of a 
method (make_sharable()?) to make the object sharable be acceptable?

>thread(const function0<void> &) should be explicit:
>
>boost::function0<void> f;
>boost::thread t;
>
>t == f;

Quite right.  I'm surprised I missed this one.  Will be fixed.

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

This seems self evident, and I don't see the standard document including 
such verbiage.

>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.

Actually, they do, but not very explicitly.  I'll clean that up.

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

*shrug*  We disagree on this point, but I'll leave that up to the decision 
of this review.

>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.'

Unfortunately, there is no "well defined behavior" here.  If an 
implementation detects this error and prevents it from happening then the 
result is not deadlock.  Explicitly stating that an implementation may 
detect it and do something (implementation defined) is still the same as 
undefined behavior.  I don't mind adding this verbiage, I'm just not sure it 
will help any.

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

Yeah, we talked about xtime.  My fault.  The addition of "is reached" is 
acceptable and I'll change that.

>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.

I guess I don't feel overly strong one way or another on this one (other 
than we're heading into the debate of whether such a concept is better or 
worse than a thread_ref concept).  What ever is decided here I'll do, but 
the examples make extensive use of this concept and I expect others will as 
well.

>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.

This "idiom" isn't really used in the standard itself and creates a 
dependency which is why I didn't include use it to begin with.  I'm willing 
to change this, however.

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

Same comment.

>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.

With the current restriction that boost::thread::join() can only be called 
once this shouldn't happen.  I guess I can think of a few corner cases where 
it could, though, so you're probably right.

>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.

Not "would" but "could".  I see the problem here though.  I'll work on this 
one.

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

Yes.  This was pointed out and I started a discussion on this topic 
intending to change this and add another FAQ as well.  Another one that I 
slipped up on.  I'll fix this.

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

Argh.  I documented these but somehow they didn't make it in to CVS or the 
submission.  I'll fix this.  Sorry about that.

Thanks for the comments!

Bill Kempf

_________________________________________________________________
Get your FREE download of MSN Explorer at http://explorer.msn.com/intl.asp