Re: Design and documentation of Boost.Threads

From: williamkempf_at_[hidden]
Date: 2001-11-12 12:19:31

--- In boost_at_y..., "Kevin S. Van Horn" <kevin.vanhorn_at_n...> wrote:
> I finally managed to eke out enough free time to finish reviewing 
and
> commenting on the documentation for the Boost threads library (as a 
prelude to
> attempting some formal correctness proofs.)  Here are my remarks.  
They
> include a mixture of comments on design, exposition, wording, and 
sometimes
> grammar.
> 
> GENERAL COMMENTS
> ================
> 
> Fairness and strongness
> -----------------------
> 
> Missing from the documentation is a discussion of the three levels 
of
> increasingly-strong guarantees for mutexes, which parallel the same 
categories
> for semaphores:
> 
> 1. Weak mutexes.  When a thread unlocks a weak mutex and there are 
other
> threads blocked waiting on the mutex, the mutex may enter the 
unlocked state
> for a short period of time until some thread locks it again.  The 
locking
> thread may not have been one of the threads waiting at the moment 
the mutex
> was unlocked.
> 
> Consider the following scenario with a weak mutex.  Thread A is in 
a loop
> where it repeatedly acquires the mutex, does something, then 
releases the
> mutex:
> 
>   while (1) {
>     <processing outside of the critical section>;
>     boost::mutex::scoped_lock l(mtx);
>     <critical section>;
>   }
> 
> If there are one or more other threads waiting to acquire the 
mutex, thread A
> may monopolize the mutex, starving the other threads so that they 
*never*
> acquire the mutex.  This can occur because thread A may reacquire 
the mutex
> after releasing it, before one of the waiting threads manages to 
acquire it.
> 
> Weak mutexes are nearly useless.  As far as I can tell, it is 
impossible to
> build starvation-free synchronization mechanisms on top of them.  
As one
> example, consider trying to build semaphores on top of weak 
mutexes.  The
> acess to the integer giving the semaphore count must be protected 
by a mutex.
> If this is a weak mutex, then a signal/up_count/V operation may 
block
> forever if there is heavy activity on the semaphore!
> 
> *** I have looked over the POSIX implementations of condition 
variables and
> the various Mutex types, and several of these are weak mutexes. ***

From a documentation stand point I can't really address this with out 
causing (theoretical at least) problems with portability.  However, 
the implementation is a definate issue.  If you have solutions for 
the implementation I'd be very interested.

> 2. Strong mutexes.  Suppose that a thread A holds a lock on the 
mutex while
> there are other threads blocked trying to acquire the mutex.  When 
A "unlocks"
> the mutex, it does not actually enter an unlocked state; instead, 
the mutex
> remains locked, but one of the blocked threads becomes unblocked 
and acquires
> the mutex.  This prevents any one thread from monopolizing the 
mutex.
> However, it is not sufficient to prevent starvation: two threads in 
a
> lock/unlock loop such as presented in the discussion of weak 
mutexes can
> starve a third by playing a game of "keep away".  That is, when 
thread A
> releases the mutex, thread B always acquires it, and when thread B 
releases
> the mutex, thread A always acquires it, leaving thread C forever 
waiting to
> acquire the mutex.

Again, I can't really mention this in the documentation.  However, 
starvation can occur even with "fair" mutexes (by one definition of 
fair).  POSIX mutexes, for instance, are open to priority starvation 
problems if you aren't careful.
 
> 3. Fair mutexes.  A fair mutex is a strong mutex with one additional
> guarantee: as long as every thread that acquires the mutex 
eventually releases
> it, any thread that is blocked on the mutex will eventually require 
it.  Note
> that mutexes that use FIFO scheduling are fair, but a mutex can be 
fair
> without using FIFO scheduling.
> 
> I seem to recall that one can implement fair semaphores on top of 
strong
> semaphores, though I do not recall the algorithm.  I'm not sure 
whether or not
> one can implement a fair mutex on top of strong mutexes.  Note the 
important
> difference between binary semaphores and mutexes: any thread can 
unlock a
> locked binary semaphore, but only the locking thread can unlock a 
locked
> mutex.  This added capability of binary semaphores is very useful 
for ensuring
> lack of starvation in synchronization algorithms.
> 
> As with mutexes, condition objects can also be strong or fair.  (The
> definition of the notify_one() and notify_all() operations rules 
out weak
> condition objects.)  Suppose that a thread A is blocked in a wait() 
on a
> condition object.  If the condition object is fair, then A can only 
remain
> blocked forever because of a lack of notify() calls on the 
condition object,
> and not because A always gets passed over in favor of some other 
thread to
> unblock on a notify_one() call.  The Boost.Threads documentation 
needs to
> specify whether or not condition objects are fair in this sense.

Again, I don't think the documentation can do this with out causing 
problems with portability.

>  Also, the
> documentation should make clear that there are two kinds of 
blocking in a
> wait() operation: a thread may be blocked on the condition object 
itself, or
> it may have been unblocked and then subsequently blocked trying to 
reacquire
> the mutex.

I'm not sure how this knowledge (which should be obvious when 
thinking about it) will be useful.
 
> If at all possible, and if the implementation cost is not too high, 
all
> mutexes and condition objects should be fair.  Otherwise you run 
the risk of
> users having mysterious, intermittent and irreproducible 
thread "lock-ups."
> If making the mutexes or condition variables fair adds a significant
> implementation cost, then Boost.Threads should have both efficient-
but-unfair
> and fair-but-less-efficient mutexes.

In general, your comments are very relevant here.  I can't document 
some of this because I need the leeway for implementing over existing 
MT libraries, but these are issues that should be addressed 
sufficiently for a standardized library (where the implementers won't 
be restricted in the same manner for the implementation).  So I'll 
need to document this stuff in some way.  I'm also very concerned 
about your findings on the implementation.  If you have any thoughts 
on how to correct these areas I'd be very interested.
 
> Handling of programmer errors
> -----------------------------
> 
> There are many places where the documentation specifies that 
exceptions *will*
> be thrown if some precondition is not met.  Do you really want to 
guarantee
> this, or do you just want to say that the exception *may* be 
thrown?  At debug
> time you do not want exceptions thrown for what amounts to 
programmer error
> (e.g., unlocking a mutex that you do not have locked), as this 
throws away a
> lot of useful information as the stack unwinds.  At debug time it 
is much
> preferable for the program to dump core so that you can examine the 
core dump
> with a debugger and have full information on the context of the 
error.  I
> recognize that a core dump is unacceptable for some production 
environments,
> so it would be nice to be able to choose between the core-dump and 
exception
> options via a compile-time flag.

Complex decision and this has been discussed a lot and honestly the 
final decision probably isn't reached yet.  Generally, the 
documentation is correct IMHO, though the implementation may want to 
assert on debug builds.
 
> In addition, I would suggest that even when you do throw an 
exception, the
> exception object should include the file and line number.  (See 
discussion
> below for overview.html.)

A useful concept for debuggin purposes, but not one that should be 
gauranteed I think.  In other words I'll consider adding this to the 
exception's what() but I think it would be wrong to put this in the 
documentation as a requirement (thinking in terms of standardization).
 
> Atomic reads and writes
> -----------------------
> 
> I think a discussion is needed of why even reads and writes of 
variables of
> primitive types may not be atomic, and that there is *one* type for 
which
> reads and simple writes are guaranteed atomic (sig_atomic_t).

Outside of the scope of the documentation, though discussion of this 
topic here and possibly in the FAQ would make sense.
 
> DOCUMENT-SPECIFIC COMMENTS
> ==========================
> 
> overview.html:
> --------------
> 
> Quote: "Priority failures (priority inversion, infinite overtaking,
> starvation, etc.)"
> 
> Comment: The programmer cannot do anything about priority 
inversion; that has
> to be addressed at the OS level.

This isn't true.  The simplest way for the programmer to avoid 
priority inversion is to never share resources between threads of 
varying priority.  This can be done by changing a thread's priority 
before and after acquiring a mutex, for instance, and in fact POSIX 
specifies a mechanism in which this can be automatically done.  This 
is an area in which Boost.Threads may need some modification, but 
even with out this support you can avoid this problem.

>  I don't believe that "infinite overtaking"
> and "starvation" are defined anywhere.

Probably not.  We'll need to add definitions.

>  I'm not sure that "priority failure"
> is an appropriate term to describe starvation.  Absence of 
starvation just
> means that a thread waiting its turn to do something does, 
eventually, get its
> turn; the concept is meaningful even in the absence of any notion 
of thread
> priorities.

I agree and disagree.  Starvation is often caused by priority 
failures.  You're quite correct that it can also be caused in other 
ways so the documentation should be made clearer here, but it's not 
strictly incorrect.

> Quote: "Every multithreaded program must be designed carefully to 
avoid race
> conditions and deadlock."
> 
> Comment: I would add starvation to the list of things to avoid.  
This is a
> commonly-overlooked error in multithreaded code.

Agreed.
 
> Quote: "Common requirements for all Boost.Threads components."
> 
> For implementors they are requirements, but for users (the great 
majority of
> those who will be reading the document) they are guarantees / 
specifications.

Hmmm... different perspective.  I'll look at this.
 
> introduction.html
> -----------------
> 
> Quote: Under "Flexiblity" it says, "When functionality might be 
compromised by
> the desire to keep the interface safe, Boost.Threads has been 
designed to
> provide the functionality, but to make it's use prohibitive for 
general use."
> 
> Comment: I have no idea what that final clause means.  Also, "it's" 
means "it
> is"; the possessive of "it" is "its." (You write "whose", 
not "who's"; "his",
> not "he's"; "theirs", not "their's; and so forth.)

Yes, that would be a typo ;).  I'm not sure how to reword the clause 
to be more meaningful, however.
 
> Quote: "Recently there has been a lot of research in other 
concepts, such as
> in 'Communicating Sequential Processes.'"
> 
> Comment: CSP is not new.  It wasn't new in in 1985, when I first 
learned about
> it.

Depends on your definition of new ;).  However, I was referring to 
new research, not to the invention of the concept.  There is a lot of 
new research for CSP that's been started recently.
 
> faq.html
> --------
> 
> Quote: "1. Are lock objects thread-safe?"
> 
> Comment: The hyphen is necessary only when the adjective 
phrase "thread safe"
> immediately precedes a noun or noun phrase.  For example, "f() is a
> thread-safe function."  The hyphen indicates grouping: "thread" 
modifies
> "safe", and not "safe function".

OK.
 
> Quote: "They are meant to be short lived objects..."
> 
> Comment: Should be "short-lived" objects: "short" modifies "lived", 
not "lived
> objects."

OK.
 
> Quote: "One easy way of doing this [locking mutexes in a standard 
order] is to
> use each mutex's address to determine the order..."
> 
> Comment: This is not portable.  The standard only allows 
comparisons other
> than == and != between pointers that point into or just past the 
end of the
> same object.  The example code for item 5, using "<" and ">" to 
compare
> mutex pointers, is not portable for this reason.  Interestingly 
enough,
> however, I seem to recall that function objects of type std::less<T 
*> are
> guaranteed to provide a complete ordering.

That's a debate I don't want to get into, because I lose every time.  
If it's enough to satisfy everyone I can simply change the code to 
use std::less.
 
> Quote: The example code for item 5 includes the lines "m_value =
> other.m_value;" and "return m_value;", which are protected by mutex 
locks.
> 
> Comment: It's worth mentioning why the protection is necessary: 
with the
> exception of the sig_atomic_t type, you don't know know whether 
reading or
> writing a value of even a primitive type can be done as a single 
atomic action
> (single memory read/write.)

There is no standard sig_atomic_t type, so this comment will just 
lead to confusion.  As it is, I think the reasons are self obvious.
 
> Quote: [Under item 6] "The internal state of mutex types could have 
been made
> mutable, with all lock calls made via const functions, but this 
does a poor
> job of documenting the actual semantics."
> 
> Comment: In fact, such a const declaration would be outright 
incorrect, since
> the logical state of a locked mutex clearly differs from the 
logical state of
> an unlocked mutex!

Depends on your definition of incorrect, which is precisely why 
things are worded the way they are.
 
> Quote: "Semaphore was removed as too error prone.  The same effect 
can be
> achieved with greater safety by the combination of a mutex and a 
condition
> variable."
> 
> Comment: This is not so easy to do correctly if you wish to avoid
> starvation!

Care to elaborate?
 
> definitions.html
> ----------------
> 
> Quote: "Thread State." [Followed by definitions of "Ready" 
and "Running".
> 
> Comment: You are getting into scheduler implementation details that 
have no
> place in a behavioral specification.  The usual way of reasoning 
about
> concurrent programs is that you assume nothing about the relative 
speeds of
> different threads, and that arbitrarily long (but finite) delays 
may occur at
> any point in the execution of a thread.  The change of state from 
Running to
> Ready and then eventually back to Running again looks just like an 
arbitrary
> pause in the execution of the thread.

I don't see any scheduler implementation details here.  There is 
certainly nothing here that assumes anything about the relative 
speeds of different threads or about delays.
 
> Quote: [Under "Race Condition"] "The result of a race condition may 
be a bit
> pattern which isn't even a valid value for the data type."
> 
> Comment: It's worth mentioning why this is so (see previous comment 
on
> atomicity.)

I think you're trying to overspecify the terms of atomicity for 
native types.  There's no knowing if a native type can be manipulated 
atomically in a portable manner and there's no standard sig_atomic_t 
type.  The description given here is accurate and complete with out a 
discussion of non-portable concerns about atomic read/write 
operations.

> Quote: "A priority failure (such as priority inversion or infinite
> overtaking)..."
> 
> Comment: Need to define priority inversion and infinite 
overtaking.  Also need
> to define and discuss starvation.

I'll look into this.
 
> rationale.html
> --------------
> 
> Quote: "However, computer science research has shown that use of 
these
> primitives is difficult since there's no way to mathematically 
prove that a
> usage pattern is correct, meaning it doesn't result in race 
conditions or
> deadlocks.
> 
> Comment: The above statement is false.  CS researchers have been
> proving the correctness of concurrent algorithms using semaphores, 
mutexes, or
> even just reads and writes of shared variables with busy waiting 
(e.g., the
> Bakery Algorithm) for decades.  It would be correct, however, to 
say that
> doing so is *difficult* for low-level synchronization constructs.

OK, some re-wording is needed here.  What was meant is that there's 
no universal formula for proving this.  Each algorithm requires 
different proofs, which, as you say, are non-trivial and difficult to 
do.

> Quote: "Though it's possible to create a lock object outside of 
block scope
> and to share it between threads to do so would not be a typical 
usage."
> 
> Comment: I would make a stronger statement: "to do so would very 
likely be an
> error." Also, you need a comma after "between threads."

OK.
 
> Quote: [Under "Rationale for Non-Copyable Thread Type".]  "... 
could provide
> more efficient reference management then wrappers."
> 
> Comment: "then" (order) should be "than" (comparison.)

Typo.  I'll fix.
 
> Quote: "Analysis of whether or not these arguments would hold true 
don't
> appear to bear them out."
> 
> Comment: "Analysis" is the simple subject, so "don't" should 
be "doesn't."

OK.
 
> Quote: "...but thread_ref no longer suffers because of it's 
reference
> management,..."
> 
> Comment: "it's" should be "its."

Yep.  My favorite typo (well, one of).
 
> mutex_concepts.html:
> --------------------
> 
> There is a systematic misuse of "concept" and "model" in this 
document.  To
> review:
> - A concept is a set of requirements on a type.
> - Mutex is a concept.
> - A type T is a model of concept C if T satisfies all of the 
requirements of
>   C.
> - The type boost::mutex is a model of Mutex.
> - Actual objects are not models of concepts nor of classes.
> 
> The phrase "mutex model" is repeatedly used when "mutex" or "mutex 
object"
> (object whose type is a model of Mutex) is the correct term.  
Sometimes "mutex
> concept" is used when "mutex object" would be correct.

Yeah, several people have been hammering this into me for some time.  
I'll look at the documentation (again).

> Quote: "A mutex (short for mutual-exclusion) concept serializes 
access..."
> 
> Comment: Concepts don't *do* anything; they're just collections of 
type
> requirements.  You mean "mutex" or "mutex object".
> 
> Quote: "A model that implements Mutex and its refinements has two 
states:"
> 
> Comment: Concept models are types, and types don't have state; 
objects do.
> Suggested rewrite: "A mutex (object whose type models Mutex) has two
> states..."   The "and its refinements" is redundant, since any type 
that
> models a refinement of Mutex also models Mutex itself.
> 
> Quote: "mutex model object" [Appears in many places.]
> 
> Comment: Replace with "mutex" or "mutex object."
> 
> Quote: "mutex model" [Appears in many places; e.g., "unlock a mutex 
model."]
> 
> Comment: Replace with "mutex" or "mutex object."
> 
> Quote: "With this pattern, a lock concept is employed where the 
lock model's
> constructor locks the associated mutex..."
> 
> Comment: "lock concept" and "lock model" should be "lock object."
> 
> Quote: "... mutex model ..."
> 
> Comment: Should be "model of Mutex" or "Mutex model", as "Mutex," 
not "mutex,"
> is the name of the concept.
> 
> Quote: "With an unspecified locking strategy, when a thread 
attempts to
> acquire a lock on a mutex model for which the thread already owns a 
lock the
> operation results in undefined behavior.  When a mutex model has an
> unspecified locking strategy the programmer must assume that the 
mutex model
> instead uses an unchecked strategy."
> 
> Comment: The model *may* use an unchecked strategy.  If the 
strategy is
> unspecified, the programmer shouldn't assume that that the thread is
> *guaranteed* to deadlock; "undefined behavior" means that anything 
can happen,
> including deadlock, aborting the program, or the computer 
going "Star Trek"
> and exploding in a shower of sparks :-).
> 
> Scheduling policies:  What is the difference between "Undefined" and
> "Unspecified"?

Not enough to be relevant, I suppose.  I'll remove Undefined.

>  Under "Priority Driven", it might be worth mentioning the
> usual way of handling priorities without starving low-priority 
threads: use
> aging, where a thread's priority increases the longer it waits, 
then goes back
> to its original level after it is serviced.

I'll look into this.
 
> Quote: [Under "Scheduling Policies," "Undefined"] "Users should 
assume that
> low-priority threads may wait indefinitely, and that threads of the 
same
> priority acquire the lock in essentially random order."
> 
> Comment:  Does "indefinitely" mean "forever", or just "an 
unspecified but
> finite amount of time"?  See also general notes on weak vs. strong 
vs. fair
> mutexes.

It means what it says ;).  There is no "forever" since the program 
will execute in a finite amount of time, but the distinction isn't 
worth arguing about.  You can't write reasonable code regardless of 
whether it's "forever" or "an unspecified but finite amount of time".
 
> Quote: [In Mutex concept, for expression "M::scoped_lock".]  "A 
type meeting
> the ScopedLock requirements.
> 
> Comment: Suggest "A model of ScopedLock."  Similar comment applies 
to TryMutex
> and TimedMutex.

OK.
 
> Quote: [In TryMutex concept.]  "A TryMutex must meet the Mutex 
requirements."
> 
> Comment: Suggest "TryMutex is a refinement of Mutex."  Similar 
comment applies
> to TimedMutex.

OK.
 
> Quote: "Boost.Threads currently supplies six classes which model 
mutex
> concepts."
> 
> Comment: Suggest "Boost.Threads currently supplies six models of 
Mutex."  In
> the table underneath this, suggest replacing "Classes Modeling the 
Concept"
> with "Models."

OK.
 
> mutex.html
> ----------
> 
> Quote: "The mutex, try_mutex and timed_mutex classes are full 
featured models
> of the Mutex, TryMutex, and TimedMutex concepts."
> 
> Comment: Somewhat redundant.  Suggest "The mutex, try_mutex, and 
timed_mutex
> classes are models of Mutex, TryMutex, and TimedMutex respectively."

OK.
 
> Table in Introduction: There is a column labeled "Implementation 
defined Lock
> Type."  This should be removed as an implementation detail on which 
library
> users should not rely.  Take a look at the description of the 
standard library
> containers; do they tell you the "implementation defined iterator 
type" for
> each container class?

Well, at one time there was a reason for documenting these 
implementation details, but since we've fully moved this into detail 
land you're right.  I'll fix this.

> Quote: "The mutex, try_mutex and timed_mutex classes use an 
Unspecified
> locking strategy."
> 
> Comment: This isn't really a strategy; it's a refusal to specify the
> strategy.  Suggest "The mutex, try_mutex and timed_mutex locking 
strategies
> are unspecified."

Well, Unspecified *was* listed as a viable strategy, but this is a 
better description any way.  I'll change it.
 
> Quote: "Programmers should assume that threads waiting for a lock 
on objects
> of these types acquire the lock in a random order, even though the 
specific
> behavior for a given platform may be different."
> 
> Comment: "Random" has probabilistic connotations.  
Suggest "Programmers should
> make no assumption as to the order in which waiting threads acquire 
a lock."
> One exception: for strong mutexes, programmers can assume that when 
a thread
> holding a mutex releases it and there are threads blocked on the 
mutex, one of
> the waiting threads acquires the lock immediately.  (At least, 
logically this
> is what happens.)

Again, I don't think I can (or should) mention strong mutexes here.  
Otherwise I agree and I'll make the changes.

> Quote: "Class mutex Synopsis"
> 
> Comment: It's worth mentioning that mutex is a model of Mutex and 
NonCopyable,
> and that the class provides no additional facilities beyond the 
requirements
> of those two concepts.  Similar comment applies to try_mutex and 
timed_mutex.

I'll look at this.
 
> recursive_mutex.html
> --------------------
> 
> The comments for mutex.html apply here also.
> 
> Quote: "Attempts to unlock them by a thread that does not own a 
lock on them
> will result in a lock_error exception being thrown."
> 
> Comment: Why not "undefined behavior"?  Do you really mean to 
*guarantee* that
> a lock_error exception will be thrown?  During debugging, I would 
prefer an
> assert() that causes a core dump over throwing an exception, as the 
latter
> loses all information as to the context in which this programming 
error
> occurred.

Complex subject.  I'll continue to look at this.
 
> lock_concept.html
> -----------------
> 
> As with mutex_concepts.html, this document systematically 
misuses "concept"
> and "model."  The phrase "lock model" should, in most cases, be 
replaced by
> "lock object."
> 
> Quote: "The lock concepts provide exception safe means for locking 
and
> unlocking a mutex model."
> 
> Comment: Concepts are specifications, not mechanisms.  
Suggest "Lock objects
> provide ... a mutex."
> 
> Quote: "Instances of lock models are meant to be short lived."
> 
> Comment: Suggest "Lock objects are meant to be short lived."
> 
> Quote: "Lock models must maintain state to indicate whether or not 
they've
> been locked and this state is not protected by any synchronization 
concepts."
> 
> Comment:  Concepts are not mechanism, and thus cannot protect.  
Suggest "Lock
> objects maintain state to indicate whether they've been locked, and 
this state
> is not protected by any synchronization mechanism."
> 
> Quote: [Lock requirements table.]
>   Expression
>   (&clk)->operator const void *()
> 
> Comment: I would suggest
>   Expression     Effects
>   clk            Convertible to void *.  Returns nonzero if ...

Hmm... I'll look at this.  Makes sense, just want to make sure I 
follow what the standard already does.

> Quote: [Lock requirements table.]
>   Expression    Effects
>   clk.locked()  Returns a bool, (&clk)->operator const void *() != 0
> 
> Comment:  I would suggest
>   Expression    Effects
>   clk.locked()  clk != (void *)0
> 
> Quote: [Lock requirements table.]
>   Expression   Effects
>   lk.lock()    Throws a lock error if locked(). If the associated 
mutex
>                is already locked by some other thread, places the 
current
>                thread in the Blocked state until the associated 
mutex is
>                unlocked, after which the current thread is placed 
in the
>                Ready state, eventually to be returned to the 
running state.
> 
> Comment:  Do you mean to guarantee that an exception will be 
thrown?  When
> debugging, I would rather have an assert() that causes a core dump, 
so that I
> can investigate the context in which the programming error occurred.
> Secondly, the wording seems to imply that when the thread holding 
the lock on
> a mutex releases it, *any and all* threads blocked on the mutex get
> unblocked.  Finally, the discussion of a Ready state brings in 
thread
> scheduler implementation details that should be of no concern to the
> programmer.
> 
> Quote: [Lock requirements table, entry for "lk.unlock()."]  "If !
locked(),
> throws lock_error..."
> 
> Comment: Do you really mean to *guarantee* that an exception is 
thrown?
> 
> Quote: "A ScopedLock must meet the Lock requirements."
> 
> Comment: Suggest "ScopedLock is a refinement of Lock."
> 
> ScopedLock Concept: If mutex m is of type M, isn't it required that 
type L and
> M::scoped_lock be the same type in the expression "L lk(m);"?
> 
> ScopedTryLock Concept: Similar comments as for ScopedLock.  Also, 
for
> lk.try_lock() the table guarantees that an exception will be thrown 
for a
> programming error.
> 
> ScopedTimedLock Concept: Same comments as for ScopedTryLock and 
ScopedLock.
> Other comments:
> - I'm uncomfortable with the use of absolute times (xtime) instead 
of
>   durations.  The xtime document refers to "the duration since the 
epoch
>   specified by clock_type."  This "clock_type" is passed to 
xtime_get(), but
>   how does the timed lock know which clock_type value to use?

Absolute times are basically required for condition variables 
(because of the looping) and so I've used absolute times for 
consistency.  This was the general consensus I got (from the 
admitedly limited responses) when I discussed this during development 
on this list.  As for the "clock_type"... there's currently only one 
valid clock_type making this a non-issue.  Things will change when we 
settle on a true Boost solution for times/durations.

> - lk.timed_lock(t) is specified to "return true if sucessful within 
the
>   specified time t, otherwise false."  You can't really guarantee 
this, due to
>   scheduler delays, or delays in acquiring the auxiliary mutex used 
in
>   the POSIX implementation of timed_mutex.  The best you can say is 
that
>   the thread won't wait forever, but will wait at least until time 
t before
>   giving up.

Neither description is fully accurate, but your is closer.  I'll work 
on this.
 
> Models table at end of document: The "Refines" column is wrong for 
every
> entry.  Every entry should have "Lock" in its "Refines" column.

Quite true.
 
> scoped_lock.html:
> -----------------
> 
> I'm not sure this document is necessary or desirable.  The C++ 
Standard
> doesn't go into separate detail on C::iterator for each container 
class C; it
> just says which concept it models, and any additional needed info 
is included
> in the description of class C.  Likewise, all of the information in 
this
> document that users should see belongs either in the description of 
the
> appropriate concept or in the description of the mutex class itself.

Another consequence of having the scoped_lock classes in a nebulas 
detail/non-detail state during development.  I'll fix this.

> The mutex_type typedef is not mentioned in the Lock concept; it 
should be.

Hmmm... probably true.  It was put there for use by boost::condition, 
but this usage is beneficial for other generic usages.  I just didn't 
think this one through.
 
> Quote: "Although this class is an implementation detail..."
> 
> Comment: Stop right there.  Implementation details don't belong in
> specifications, except as examples clearly labeled as possibilities 
only.

The idea was that developers could use the templates for the Lock 
concepts in their own Mutex refinements.  However, the types have now 
been fully moved into detail land and should simply not be documented.
 
> Quote: [For "operator const void * () const"] "If the associated 
mutex is
> currently locked, a value convertible to true, else a value 
convertible to
> false."
> 
> Comment: Don't you mean "If the associated mutex is currently 
locked BY THIS
> OBJECT..."  Also, void * values are NOT convertible to bool, 
although they can
> be used as the test of an if, while, or for, and can be used as an 
argument to
> &&, ||, and !.

Correct on both counts.
 
> Quote: "A const void * conversion is considered safer than a 
conversion to
> bool."
> 
> Comment: You should explain why.

This is the rationale, and re-iterating this rationale every time 
it's used is needless IMHO.  However, it's a non-issue when I remove 
this documentation ;).
 
> scoped_timed_lock.html
> scoped_try_lock.html
> ----------------------
> 
> See comments for scoped_lock.html.
> 
> 
> condition.html
> --------------
> 
> Quote: "...a mutex object modeling a Mutex concept."
> 
> Comment: Types model concepts; objects do not.
> 
> Quote: "...a lock object modeling a Lock Concept..."
> 
> Comment: Types model concepts; objects do not.
> 
> Quote: "The mutex must be locked prior to waiting on the condition,
> which is ensured by passing a lock object modeling a Lock Concept to
> the object's wait functions."
> 
> Comment: This doesn't ensure that the mutex is locked, although it 
does ensure
> that the mutex gets unlocked.

Actually, it does ensure it, through a call to locked() on the lock 
object passed in.

> Quote: "While the thread is waiting on the condition object, the 
mutex
> associated with the lock is unlocked."
> 
> Comment: The use of "While..." would seem to indicate that "is 
unlocked" is
> not the passive form of "lock", but a simple statement of the state 
of the
> mutex.  That is, the sentence seems to say that the mutex remains 
unlocked
> during the entire time that the thread is waiting on the condition 
object.
> Suggest "Upon blocking on the condition object, the thread 
unlocks/releases
> the mutex."
>
> Quote: "When the thread returns from a call to one of the condition 
object's
> wait functions, the mutex is again locked."
> 
> Comment: Does not make clear who locks the mutex, and is not clear 
on whether
> this happens just before or just after the call.  Suggest "The wait 
functions
> reacquire the mutex just before returning."

I'll work on this.

> Quote: "The tricky lock/unlock/lock sequence is performed 
automatically by the
> condition object's wait function."
> 
> Comment: Only the unlock/lock part is performed by the wait 
function.

True.  This description needs some cleaning up.
 
> Quote: "notify_one()... Effects: If there is a thread waiting on 
*this, change
> that thread's state to ready."
> 
> Comment: This is not completely accurate.  After being unblocked, 
the thread
> then has to reacquire the mutex, which may cause it to block 
again.  Also, the
> issue of fairness (lack of starvation) needs to be addressed: in 
the case
> where some thread t is waiting on a condition and there are always 
multiple
> threads waiting when a notify_one() occurs, is there any guarantee 
that
> eventually thread t will be chosen, or may thread t *always* be 
passed up in
> favor of another thread and wait forever on the condition?  (I'm not
> considering here the issue of subsequently reacquiring the mutex.)

The re-locking semantics are covered by the documentation of the wait 
methods.  I'm not sure that changing the wording here would be 
useful.  And again, I'm not sure that mentioning the fairness is 
possible here, and even with fair implementations you can get the 
same problems, as illustrated by starve_phil.

> Quote: "void wait(ScopedLock & lock); ... All effects occur in an 
atomic
> fashion."
> 
> Comment: This is not and cannot be true.  The initial 
unlock/suspend can be
> done atomically, but becoming unblocked is a two-stage process: (1) 
the thread
> gets awakened by some other thread calling notify_one() or 
notify_all(); (2)
> the thread reacquires the mutex.  For conditions, we really need to
> distinguish two separate blocking states: blocked on the condition 
itself, and
> blocked on the mutex while attempting to reacquire the mutex.

Technically accurate, but I'm not sure the distinction is useful.
 
> Quote: "timed_wait ... Throws: lock_error if !lock.locked()"
> 
> Comment: Do you mean to guarantee that an excpetion is thrown?

This is what ensures the mutex is locked ;).  Seriously, though, I'm 
still thinking about this issue.
 
> Quote: "bool timed_wait(ScopedLock & lock, const xtime & xt);"
> 
> Comment: See my previous reservations about using an absolute time 
instead of
> a duration.

The rationale for absolute times is well documented by POSIX.  Usage 
of durations for timed waits on conditions is very problematic given 
the presence of "spurious wakeups" and the need for checking the 
predicate in a loop.  Durations require re-calculations through each 
loop iteration which complicates the code and can effect the over all 
performance.
 
> Quote: "timed_wait ... Returns: false if xt is reached, otherwise 
true."
> 
> Comment: You can't really guarantee this.  See my previous comments 
on
> ScopedTimedLock.
> 
> 
> thread.html:
> ------------
> 
> Quote: "This includes completion of all thread cleanup handlers,..."
> 
> Comment: What thread cleanup handlers?  Boost.Threads has no such 
notion.

A misuse of the term for those knowledgable in POSIX.  I meant thread 
specific storage cleanup handlers.
 
> Quote: "if a thread object is destroyed without join() having first 
been
> called, the thread of execution continues until its initial function
> completes."
> 
> Comment: This sounds scary.  Another possibility is to have the 
thread dtor
> call join().  If you think the existing semantics are best, it 
would be nice
> to have a section in rationale.html explaining your reasoning.

This is the POSIX concept of a detached thread.
 
> Quote: "thread(const boost::function0<void> & threadfunc);"
> 
> Comment: I would like to see a hyperlink here to a description of 
the
> function0 template.  I am not familiar with the function0 template, 
so I have
> to ask: does the const prevent me from using a function object 
whose state is
> intended to be modifiable by the newly-created thread?

Yes and no, with the emphasis on yes.  This is an error in the 
implementation.
 
> Quote: "Comparison Operators..."
> 
> Comment: Gives requirements on *this (left-hand side of ==), but 
not on rhs.

Hmm... I'll fix this.

> Quote: "static void sleep(const xtime & xt);"
> 
> Comment: As previously mentioned, I have doubts about the use of 
absolute time
> instead of duration.  Again, how does sleep() know the appropriate
> clock_type/epoch to use?
> 
> 
> thread_group.html:
> ------------------
> 
> Quote: "void remove_thread(thread* thrd);
>         Effects: Removes *this's list of managed thread objects.
>         Throws: ? if thrd is not it *this's list of managed thread 
objects."
> 
> Comment: What is the "?" for?

This was missed during documentation.  I'll fix this.

>  For effects, don't you mean "Removes thrd from
>  *this's list of managed thread objects"?

Yep.
 
> call_once.html
> --------------
> 
> Why does call_once() take a void (*func)() argument instead of a 
function
> object?

Originally, for safety (constructing the function object could have 
synchronization issues as well).  I expect I'll change this, though.  
It's been brought up before.
 
> thread_specific_ptr.html
> ------------------------
> 
> Quote: "Note: There is an implementation specific limit to the 
number of
> thread specific storage objects that can be created, and this limit 
may be
> small."
> 
> Comment: This note is not very useful without *some* way of knowing 
what the
> limit may be.  Perhaps there should be a const static value giving 
the limit,
> which will vary from one platform to another.  It would be nice to 
know a
> lower bound on this limit (i.e., you're guaranteed at least 5 
thread-specific
> pointers), and it would be nice to know the limit (or a reasonably 
tight lower
> bound) on the most common platforms.

There's a limit on memory but we get along well with out a means to 
tell how much memory is left for our use.  There's also the issue 
that on Win32 the number depends on the actual OS (which can be 
worked around with runtime checks).  Then there's the fact that we're 
considering an implementation that limits this only by the amount of 
available memory.  This is something I've thought about but left out 
in the initial release for all of these reasons.
 
> Quote: "Destructor... Does not destroy any data that may be stored 
in any
> thread's thread specific storage.  For this reason you should not 
destroy a
> thread_specific_ptr object until you are certain there are no 
threads running
> that have made use of its thread specific storage."
> 
> Comment: This sounds unsafe and prone to resource leaks.

But it's also all that POSIX guarantees.  In fact, it's recommended 
you never call pthread_key_delete in portable programs by many books 
for this very reason.  This may all change at some point if we change 
the implementation but for now this is the gaurantee.

>  A rationale is
> needed here for why you consider this better than having 
thread_specific_ptr
> be like auto_ptr or scoped_ptr (dtor automatically deletes pointer).

I don't consider it better, I just consider it the best gaurantee 
given by the current implementations.  Until a final decision is made 
here I'm not sure that rationale will help anyone.
 
> xtime.html
> ----------
> 
> Why not provide the obvious two-argument ctor for xtime?  It would 
still be
> compatible with the C version of xtime.

Because I find it highly unlikely that the C++ standard would do 
this.  I instead expect we'll either replace xtime entirely with a 
C++ specific time concept or that a C++ specific time concept will 
have a conversion (explicit or not) to xtime.  In other words, I 
don't expect a C++ programmer to use xtime with out some sort of OO 
wrapper, if he'll use it at all.

Don't get too hung up on xtime.  It's there solely as a placeholder 
until Boost can decide upon a direction for time/duration handling 
concepts.

> Quote: "xtime_get(struct xtime * xtp, int clock_type);"
> 
> Comment: It's not crystal clear what clock_type is.  I gather that 
it's
> supposed to be one of the enums in the synopsis, but this isn't 
explicitly
> stated anywhere.

You're correct.  I need to work on this a bit.
 
> lock_error.html
> ---------------
> 
> Quote: "class lock_error : public std::runtime_error"
> 
> Comment: A lock_error is not a runtime error.  It is an error which 
it is
> reasonable to expect the programmer to avoid.  Since it is a 
programmer error,
> it should derive from std::logical_error.

I can buy that.
 
> I would recommend that exceptions indicating programmer errors 
include the
> file name and line number where the error was detected, to aid in 
debugging.
> Example:
> 
>   class lock_error : public std::logical_error
>   {
>     char const * file_;
>     char const * line_;
>   public:
>     lock_error(char const * file, char const * line)
>     : logic_error(string("Lock error at file: ") + file + ", 
line: " + line)
>     { }
>     char const * file() { return file_; }
>     char const * line() { return line_; }
>   };

Storing the line as a seperate string is strange.  If it were to be 
stored sperately I'd store it as an integral type.  However, this 
whole concept is foreign to the design of the C++ standard and would 
be considered an implemtation detail useful for debugging.  So I'll 
consider adding this info to the what() string, but I'm not sure it 
should be documented as a requirement of the implementation.
 
>   ...
>   throw lock_error(__FILE__, __LINE__);  // Perhaps define a macro
>   ...

And this illustrates why C++ wasn't designed this way ;).
 
> config.html
> -----------
> 
> You should make clear whether these macros are intended to be used 
as
> 
>   #ifdef MACRO_NAME
> 
> or
> 
>   #if MACRO_NAME

That would depend solely on the code using them, no?  Seriously, 
though, I think this is fairly obvious from the macro names, given 
they are all BOOST_HAS_* variants.

Thanks for your comments.  They are definately useful.

Bill Kempf