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
  jinmei                             |                Status:  reviewing
                       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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 So, I took the review because it was there for some time already,
 unattended.

 The suggestion (to pass `Locker` instead of `Mutex` if it looks more
 explicit) is still valid.

 Also, some more suggestions and questions to the code.

 First one, do we want `signalAll` too? It would be a simple addition, but
 we probably don't need it right now.

 In the tests, the `signalAndWait` function probably should be moved into
 the
 `#ifdef DEATH_TEST`. If there's a system without death test support, the
 function could generate an unused function warning.

 The changes of `Locker` make it possible to hold the `mutex_` as reference
 instead of pointer. Maybe it would be cleaner that way.

 This method, can it ever happen that the locked count is bigger than 0
 when we just locked?
 {{{#!c++
 Mutex::postLockAction() {
     if (impl_->locked_count != 0) {
         isc_throw(isc::InvalidOperation, "Lock attempt for locked mutex");
     }
     ++impl_->locked_count;
 }
 }}}

 And the other pair one, can it happen to throw in destructor of `Locker`?
 {{{#!c++
 void
 Mutex::preUnlockAction() {
     if (impl_->locked_count == 0) {
         isc_throw(isc::InvalidOperation, "Unlock attempt for unlocked
 mutex");
     }
     --impl_->locked_count;
 }
 }}}

 Is this really true it is allowed to pass unlocked mutex to
 `pthread_cond_wait`? I mean this comment:
 {{{
 Unlike \c pthread_cond_wait(), however,
 this method requires a lock for the mutex has been acquired.
 }}}

 According to my man page, it is not forbidden explicitly, but it looks to
 be in a gray area at least, or forbidden implicitly:
 {{{
 These  functions  atomically release mutex and cause the calling thread to
 block on the
        condition variable cond;
 }}}

 To release the mutex, it must have been locked first, so I understand it
 in a way that even if it doesn't break (in case of the
 `pthread_cond_wait`), it is not desired and works by accident.

 Also, there are few typos:
  * `pthread_bond_` should be `pthread_cond_`.
  * „destructed” ‒ isn't destroyed better?
  * This one „waits“ should probably be „wakes“:
 {{{
 This method waits one of other threads (if any) waiting on this object
 via the \c wait() call.
 }}}

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