Re: Review: Boost.Threads

From: williamkempf_at_[hidden]
Date: 2001-09-10 22:00:02

--- In boost_at_y..., "Ed Brey" <edbrey_at_y...> wrote:
> From: <williamkempf_at_h...>
> 
> > > semaphore: When would up() ever be called such that it would 
raise
> > count
> > > above max?  I would have expected count <= max be a 
precondition to
> > > calling up().  There's probably a good reason that I'm not think
> > of.  It
> > > might be good to document what it is.
> >
> > I was just (inconsistently with other areas I admit) avoiding
> > undefined behavior.
> 
> For such a low-level class, I'm not sure how avoiding undefined 
behavior
> by a run-time check is a good idea.  It seems similar to how it 
would be
> if std::stack().pop() had a defined behavior.  One would be paying 
for
> something that is never used.

But you only pay in theory (or more accurately you only pay on some 
platforms).  The cost is also extremely small for most 
implementations.  I also expect that this could be an easy error to 
make, giving us some benefit in having a defined behavior.  There may 
even be a use for this beyond what I'm thinking.

I'm not hard and fast on this one, though.

> > This debate doesn't have a clear winner in my mind.  Some argue 
for
> > free functions and others argue for members, and the reasoning is
> > sound for both sides.  I personally prefer members, even if they 
are
> > static.  It's a clearer OO design, with functionality clearly
> > encapsulated in the object's interface.
> 
> In this case, it is exactly the cleanliness of the OO design that I 
find
> the member functions break.  The reason is that the wait functions 
are
> not primarily acting on the object, but rather on the current 
thread.

We disagree here.  To me you are waiting entirely on the condition, 
it's just a side effect that the current thread blocks.  Let us take 
your viewpoint for granted though and carry it to its final 
conclusion (I cheated and read some of the other posts on this 
thread, so I'll reference things said later).

You contend that the action is on the current thread, not the 
condition.  I'd still not opt for a free function since I truly think 
this does a poorer job of self documenting, so I'd elect to make this 
a static method on thread.  So we'd have thread::wait() with several 
overloads for the condition.

Later you argue that sleep() fits this bill as well because it blocks 
the thread as well and we are simply waiting on the thread.  I 
disagree that sleep() fits this concept which I'll explain later, but 
this claim helps us to further feel out this idea.  When you call 
semaphore::down() you also cause the thread to wait, so by the same 
logic it should be removed and replaced with yet another overload to 
thread::wait().  The same can be said for mutexes (ignoring our 
scoped_lock concept which makes the usage safer).  Basically, ALL 
synchronization objects should have an overloaded thread::wait().

This all seems logical, and in fact that's the design used by Win32.  
In fact, Win32 conveniently makes all synchronization 
types "waitable" types allowing them to have a single polymorphic 
method to wait on all of them.  It seems logical and consistent.  
However, the idea falls apart when you realize that all of these 
synchronization objects have a converse operation to the wait().  
Unfortunately the converse operations all have different names, 
showing the actual failing in this whole logic.  The name release() 
will work for many of them, but not for all, and frankly the term can 
lead to confusion.

I don't like this idea at all.  The names become too generic and the 
concepts lose meaning.  You can argue that I carried the idea to far, 
but if you go back to just the condition I still have problems seeing 
how this version of "wait" is different from the other versions 
of "wait" for the other synchronization objects.  For example, if 
one "locks" the mutex one also "waits" on the condition.  The object 
of the verb in both cases is the synchronization object, not the 
thread.

As for changing the name of sleep() to wait()... it's not orthogonal 
with other waits.  With other waits there's something that you're 
waiting on and an (optional) timeout.  With sleep() there's nothing 
that you're waiting on.  If it were renamed to wait() you'd be 
waiting on nothing with a timeout, which I'd say should return 
immediately with out blocking.

> [Including Windows.h in header]
> > It's more a problem with macro clashes caused by Windows.h and
> > portable code.  I'm not likely to budge on this, since inlining 
makes
> > little difference in performance here.
> 
> Understandable.  Just out of curiosity, what kind of applications 
are
> out there that are compiled on win32, use threads, and don't alreayd
> include <windows.h> for something else.

Beman gave a great example of this.
 
> > > In condition::notify_all, line 125, signals is assigned with no
> > effect.
> >
> > I'm not sure I follow.
> 
> The line is:
>         m_waiting += (signals = m_blocked);
> 
> signals is a local built-in variable that is never again referenced 
in
> the function.

I'll have to look closely at this.  It's probably dead code left over 
during one of the refactorings.
 
> > > The enumeration of the various locking strategies and scheduling
> > > policies do an excellent job of putting the nature of boost
> > unspecified
> > > policies in a clear light.  The only exception is that the 
strategy
> > on
> > > unlocking is ambiguous.  Only the checked strategy mentions 
this.
> > > Recursive, unchecked, and unspecified are silent.  I would have
> > expected
> > > at least the last two, if not all three remaining to not check, 
i.e.
> > > make m_locked == true a precondition.  Currently, it is not a
> > > precondition, as the code for scoped_lock::unlock always checks
> > whether
> > > the lock is locked, even though the policy is "unchecked".  This
> > seems
> > > inconsistent.
> >
> > There's a distinct difference between the "locked" state of the
> > scoped_lock and the "locked" state of the mutex.  The policies 
refer
> > to the locked state of the mutex.
> 
> I don't understand the distinction.  Can you point me to somewhere 
where
> I may educate myself.

There can be multiple locks on a single recursive mutex.  So the 
scoped locks maintain a lock state in order to determine whether or 
not the mutex needs to be unlocked during destruction and each mutex 
keeps it's own state that actually indicates whether or not it is 
locked.  If the lock object has a locked state then you know the 
associated mutex has a locked state as well, but if the lock object 
is not locked you don't have any idea about whether or not the 
associated mutex is locked.  Since lock objects have to maintain this 
state the checks are "free" and so the exceptions help us to catch 
errors.  Such checks are not necessarily free for mutexes, however, 
so it's only recommended that they throw when errors in usage occur.

Bill Kempf