[boost] Threads Review

From: David Abrahams (dave_at_[hidden])
Date: 2002-10-16 13:12:37

I realize this is almost a year too late ;-), but I've been reviewing
the Boost.Threads documentation in detail and I have the following
comments. I also have a whole lot of editorial changes to suggest,
which I'll post separately as a patchset.

* "Thread-safe": this is a term which is well-defined for programs. It
  is /not/ well-defined for classes or functions, though the
  documentation uses it that way repeatedly. The definition
  (definitions.html) says:

      "while some thread-safety violations can be determined
      statically at compile time, many thread-safety errors can only
      be detected at runtime"

  Reasoning/justification, please? I refuse to believe that there
  exist thread-safety problems which can't be detected via careful
  static analysis of the system without proof. Maybe you mean to say
  that some kinds of mistakes can be prevented with compile-time
  constructs, but some mistakes are logic errors which compilers have
  no way to detect?

* Thread State - I think the thread state distinction between "ready"
  and "running" is a false one, since the system can move a thread
  from "ready" to "running" at an arbitrary time, without provocation,
  and the formal description of the library would be a lot simpler if
  you just had a "running" state.

  Also, you use the term "processor" which is not defined either in
  Boost.Threads or in the C++ standard. It is also not detectable by a
  user program AFAICT (by time-sharing/context-switching, an OS can
  implement as many "virtual processors" as it wants), so I suggest
  it's meaningless and should be stricken.

* The definition of "Starvation" gives me pause. Is there any way to
  be more-precise about what's meant by "sufficient"?

* The definition of "accessible from multiple threads" is
  unintuitive:

        "An object [1.8, 1.9] is accessible from multiple threads if
     it is of static storage duration (static, extern) [3.7.1], or if
     a pointer or reference to it is explicitly or implicitly
     dereferenced in multiple threads."

  It would be better to change "is explicitly or implicitly" to "can
  be". It would be even better to say something like:

        "An object [1.8, 1.9] is accessible from multiple threads if
     it is stored in the function object passed to a thread's
     constructor, it is of static storage duration (static, extern)
     [3.7.1], or if a pointer or reference to it is accessible from
     multiple threads."

* The second and fourth rows of the table in definitions.html seem
  very wrong to me. To review: [read to the end before replying... I
  may come to some realizations by the end]

    For the same row of the table, the value of an object accessible
    at the indicated sequence point in thread A will be determinate
    and the same if accessed at or after the indicated sequence point
    in thread B, provided the object is not otherwise modified. In the
    table, the "sequence point at a call" is the sequence point after
    the evaluation of all function arguments [1.9/17], while the
    "sequence point after a call" is the sequence point
    after the copying of the returned value...

                      mutex m;
                      int shared_counter = 0;

    Thread A                               Thread B
   +-------------------------------------+------------------------------+
   | The sequence point at a call to a   | The sequence point after a   |
   | library function which locks a      | call to a library function   |
   | mutex, directly or by waiting for a | which unlocks the same mutex |
   | condition variable.                 |                              |
   +-------------------------------------+------------------------------+

                                           scoped_lock lkB(m);
                                           ++shared_counter;
                                           lkB.unlock(m);
     scoped_lock lkA(m);                   
     int x = shared_counter;

   Hmm, on closer inspection, this one is OK... though I'd be a lot
   more comfortable if the Thread A cell said "after a call" instead
   of "at a call". In practice, the only reason you can say "at a
   call" is because it's impossible to squeeze anything in before the
   locking function gets called and after the evaluation of all
   function arguments. And that's prone to confusion, also, since you
   can build expressions which call several functions and so contain
   several such sequence points

   ...but thinking about this some more, I'm not sure it has any
   meaning for a user. You're telling him that after evaluation of all
   arguments, but before the locking function is called, the value of
   some shared data will be determinate. But as I said in the
   paragraph above, the user can't touch the data there, since there's
   no way to insert anything between that touches the shared data at
   that point. I don't think your wording makes it OK to do the
   assignment in the last line, even though that seems like what you
   meant.

   +------------------------------+-----------------------------------+
   | The sequence point at a call | The sequence point after the call |
   | to a library function which  | to the library function which was |
   | signals or broadcasts a      | waiting on that same condition    |
   | condition variable           | variable or signal                |
   +------------------------------+-----------------------------------+

   First of all, the terms "signal" and "broadcast" are not defined
   anywhere. Do you mean "calls c.notify_one or c.notify_all" for a
   condition c? If so:
                        condition c;
                                                c.wait(lkB);
     c.notify_one()                             ++shared_counter;
     int x = shared_counter;                    ++shared_counter;
     
   The same problems about the meaningfulness of the claims exist
   here, but if I stretch your wording to cover what I think you meant
   in row 2, it definitely doesn't make the assignment OK. Worse, I
   think it's even a false claim at the undetectable point before
   notify_one() is entered, since some thread C can call
   c.notify_one(), which allows threads waiting on c to proceed.

   Another thing to notice about this row. It's got a kind of
   mirror-image relationship to row 2, where it's thread A that's
   doing the waiting. Maybe you somehow transposed the columns?

   Perhaps I've misunderstood the whole thing, but if so I think it
   at least needs some serious rewording for comprehensibility.

   Hmm, now maybe the crux of this is: 

      "the value of an object accessible at the indicated sequence
       point in thread A will be determinate and the same if accessed
       at or after the indicated sequence point in thread B"
                                                ^^^^^^^^^^^

   I took this to mean that the sequence point was in thread B, but
   the access was in thread A.

   Did this actually mean:

      "the value of an object accessible in thread A at the indicated
       sequence point will be determinate and the same if accessed in
       thread B at or after the indicated sequence point"??

   If so, I have to re-evaluate the table. The above interpretation
   makes row 2 nonsensical, unless we make "unless otherwise modified"
   mean that thread A is not allowed to modify the variable after the
   mutex is locked. I think I'd have to make the same assumption if I
   want row 4 to make any sense. That also goes for row 1. OK, this
   must be the right interpretation. Right?

Well, this was so long that I'll have to continue in a followup
message.

--
           David Abrahams * Boost Consulting
dave_at_[hidden] * http://www.boost-consulting.com