Re: [boost] RE: Review: Boost.Threads

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

From: "William Kempf" <williamkempf_at_[hidden]>
> 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).

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

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

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

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

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

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

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

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

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

--
Peter Dimov
Multi Media Ltd.