BIND 10 #2332: define and implement wrapper interface for conditional variables

[BIND 10 Development]

#2332: define and implement wrapper interface for conditional variables
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:
  jinmei                             |                Status:  new
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121023
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  b10-auth                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  background zone loading            |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:7 vorner]:

 > Are there two mutexes in the game (internal and the passed to wait) or
 just
 > one? If just one, how does the caller lock? If two, how do they
 interact? And

 There are two mutexs: one within `CondVar` (hidden from others), the
 other passed to `CondVar::wait()`.  The former controls that the lock
 for the latter will be held immediately before the thread sleeps on
 `CondVar` (and prevent deadlock situation).  The latter is used to
 protect some other data.  btw, this design is derived from
 boost::condition_variable (but much simplified).

 > if they interact, is it guaranteed to be race-condition-free and
 deadlock-free?
 > Don't forget, the mutex is not for the protection of the condition
 variable, it
 > is for the shared state around it. The whole trick with condvars is that
 the
 > wait unlocks the mutex protecting the data atomically. Also, can
 multiple
 > threads wait on the same condition variable? I'm not saying your
 approach is
 > necessarily wrong, but it's not obvious it works and would need some
 kind of
 > proof.

 I understand these.  The second mutex (passed to wait()) is for the
 shared state around the condvar.  The internal mutex within `CondVar`
 is a tool only needed within `CondVar` to provide compatible interface
 as pthread_cond_wait().

 The question on race condition and deadlocks is valid.  I thought
 about it and concluded it was safe, but as always for thread related
 things I admit it's quite possible I still miss something.

 > Also, you don't need to lock for signal, it's save in either state of
 the mutex
 > (in my understanding, there's only difference in performance and it is
 case by
 > case dependant on which mode of operation is faster).

 I understand this too.  That applies to the second mutex (passed to
 wait()).  The mutex within `CondVar` needs to be held for the internal
 purpose of of the `CondVar` implementation (without this a deadlock
 can happen).

 > But what scares me is the shared pointer with the locker. The locker is
 meant
 > to be generally allocated on stack, for obvious reasons. The approach
 with
 > dynamic allocation has disadvantages in both less convenient operation
 and
 > being more error-prone. I think you're trading this in favor of not
 having a
 > friend declaration. I'd rather have the complexity hidden inside than
 passing
 > it onto the caller.

 I understand the use of the (shared) pointer is scary.  But my main
 reason for the use of it is to avoid making `Mutex::(un)lock`
 completely public to applications (which itself was not part of my
 design but I thought that was a design concept of the `Mutex` class),
 not to avoid making `CondVar` a friend of `Mutex`.

 Consider how we use the condition variable in our "data source
 configurator" thread.  Conceptually it would look like this:

 {{{#!cpp
 void
 run() {
     // some kind of mutex to protect the command queue, shared with the
 main
     // thread.
     Mutex mutex;
     CondVar condvar;            // shared with the main thread

     while (true) {
         mutex.lock();
         while (queue.empty()) {
             condvar.wait();
         }
         // at this point mutex is locked and queue is not empty
         command = queue.pop();
         mutex.unlock(); // release it as the following would be time
 consuming

         // do something based on command: reload a zone, reconfigure
 entire
         // data source clients, or shutdown the thread, etc.
     }
 }
 }}}

 But we cannot use our `util::Mutex` directly as `Mutex` in the above
 conceptual code because in our version lock() and unlock() are private
 methods.  It also didn't seem to be easy to use `Mutex::Locker` in the
 above scenario because, just as you mentioned, it would normally be
 instantiated on stack.

 I didn't think it possible to resolve this situation through making
 `CondVar` a friend of `Mutex`, hence the introduction of pointer to
 `Locker`.  With that introduction `CondVar` simply didn't have to be a
 friend of `Mutex`, and that's why I didn't make it so.

 I see the concerns you had in this proposal, and I don't think this
 one is the best one in terms of complexity and understandability.  If
 there's a design to make it possible in a safe and more
 understandable, I'm happy to use it.  Do you have a concrete proposal?

 And, one more thing: I also tend to agree that using `Locker` through
 a pointer could be considered a kind of abuse, but if so, I think the
 documentation should state that more clearly.  It currently reads:

 {{{#!cpp
     /// If you create the locker on the stack or using some other "garbage
     /// collecting" mechanism (auto_ptr, for example), it ensures
 exception
     /// safety with regards to the mutex - it'll get released on the exit
     /// of function no matter by what means.
 }}}

 This looks like it's actually expected to be used through auto_ptr,
 even though it's not explicitly encouraged.  If such usage is rather
 discouraged IMO we should say something like "even if something like
 this is possible, it's generally discouraged".

-- 
Ticket URL: <http://bind10.isc.org/ticket/2332#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development