Re: Review: Boost.Threads

From: williamkempf_at_[hidden]
Date: 2001-09-01 15:17:15

--- In boost_at_y..., "Peter Dimov" <pdimov_at_m...> wrote:
> From: "William Kempf" <williamkempf_at_h...>
> > From: "Peter Dimov" <pdimov_at_m...>
> > >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).
> 
> I'm wearing my 'pedantic reviewer' hat now (it's as if I see the 
submission
> for the first time.) Reviewing an incomplete design is dangerous. 
If I were
> to take the submission at face value, my comments would be:
> 
> "The boost::thread default constructor should be removed. There are 
several
> ways to invoke undefined behavior using a default-constructed 
object and too
> few legal operations; the only operation that makes sense that I 
see is t ==
> thread(), which could have easily been provided in the (more 
efficient)
> t.is_current() form."

I'm not sure that would be more efficient, but otherwise I understand 
what you're saying.  There's no doubt that we need further 
functionality for boost::thread and with out this added functionality 
your points stand here.  The additional functionality is going to 
entail a lot of design work, though, and there's been pressure to get 
at least a minimal library placed into Boost.  I may not have made 
the right decision here, but that is the decision that was made.

> > >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 C-style cast is, unfortunately, valid, even when the base class 
is
> inaccessible. (5.4/7)

Hmmm... ugly.  Can't say as I understand the reasoning for this.  In 
any event, the whole point of boost::noncopyable is to be used in 
private derivation in this manner, so I wouldn't expect much 
confusion with things as they are now.  I will, however, add the 
comment.
 
> > >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.
> 
> This is probably a nit; I usually consider the Introduction section
> 'non-normative' (explains but does not strictly define) and the 
Reference
> section 'normative' (strict definitions, no human-readable 
explanation.)

I'll see if I can do something about this one.
 
> > 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.
> 
> Yes, it is outside the mutex scope. I'm mentioning it here for a 
lack of
> better place. I think that the library should provide support for 
ordered
> locking for two or more "mutex models," since this functionality is 
critical
> for deadlock prevention.

I mostly agree.  The concept you've given won't always work (you 
can't always acquire all the mutexes at once) and there are other 
methods for avoiding deadlock (including the try_lock concept that is 
included).  Eventually we will need a concept such as this, it's just 
a higher level concept than what's needed for this submission.  As 
long as the FAQ warns of the dangers and how to avoid it (only 
newbies will not already know about this) then it's trivial enough to 
avoid this problem with a pattern instead of a concrete concept model.

> > 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.
> 
> True, but tweaking the cancelability state is less elegant. In the 
general
> case, you need to query the previous state, set it to disabled, 
lock the
> mutex, restore the previous state. Which leads to problems if 
another thread
> has altered the cancelability state between the save and the 
restore.

Actually, changing the state is *more* elegant.  During a destructor 
you'll want all cancellation points ignored, not just a call to lock 
a mutex.  If a non-cancelling variant is needed for every 
cancellation point then the library becomes more difficult to 
understand as you have numerous seemingly identical methods.  The 
problems you refer to (restoring the previous state even in the face 
of exceptions) are better solved through a RAII concept for disabling 
cancellation.

> Of course this is not a showstopper for the current submission, 
it's simply
> something to keep in mind.

I've been thinking a lot about cancellation.  I think I've got a 
fairly good idea how to design this.
 
> > >I think that adopting standard 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.
> 
> Name debates are fun. I'm not a big fan of join/signal/broadcast 
myself, but
> they have a clear advantage over the alternatives -- you can explain
> objectively why they were chosen.

With join you can, I'm not so sure about signal/broadcast.  These 
aren't "universally" accepted names for the operations like join is.  
I can objectively explain why notify_one and notify_all were chosen, 
even if I'd be just as happy (maybe happier) with signal/broadcast.

> > >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.
> 
> There is a difference between 'undefined behavior' 
(1.3.12), 'unspecified
> behavior' (1.3.13) and 'implementation defined behavior.' (1.3.5)
> 
> 'Undefined' means no guarantees at all.
> 'Unspecified' means that the implementation must choose a behavior, 
but is
> not required to document the choice.
> 'Implementation defined' means that the implementation must choose a
> behavior and document the choice.
> 
> So 'implementation defined' is more strict than 'undefined.'

Yes, but...

a) For a portable use you have to treat all three types the same.
b) I'm uncertain as to whether POSIX or Win32 gaurantee any behavior 
here.

However, for the standard you are quite right about all of this, so 
I'll add the verbiage.

> To return to our 'join' example, if you remove the *this != thread()
> precondition, the behavior of a thread joining itself is well 
defined. The
> thread blocks forever. This may be preferable to 'undefined 
behavior' (that
> on our particular implementation may mean rebooting the machine 
without
> flushing the disk cache, for instance.)

I don't gaurantee that the thread will wait for ever, and your 
suggestion to make it "implementation defined" wouldn't gaurantee 
this either.  I agree that the verbiage should be added to strengthen 
the contract, but I do not agree that the result is "deadlock".

Bill Kempf