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

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:16 vorner]:

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

 I'm not sure about this part.  Is this some suggestion to the
 implementation or general discussion?

 > 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 general I prefer the YAGNI principle in these cases.  Assuming
 that's okay, I've clarified that in the documentation.

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

 Ah, good point, and, actually I'd rather use EXPECT_DEATH_IF_SUPPORTED
 than ifdef so that the code is at least always built.  IF_SUPPORTED
 seems to be available since gtest 1.4 (which is reasonably old), so I
 guess we can safely assume the availability.  For now I've changed it
 using IF_SUPPORTED.

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

 Changed to a pointer.  Actually, the use of mutable reference seems to
 be a controversial point (with a mutable references the code could be
 more confusing), and, while I myself generally prefers references
 because it eliminates NULL cases, I also see the point against mutable
 references.  So, in this case I'm not sure which one is actually
 better, but I accepted the suggestion anyway.

 > 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;
 > }
 > }}}

 I'm not sure if I understand the comment...do you mean, e.g., this
 should be impossible by the API contract and can be assert()?
 Perhaps, if this code is only active in the debug mode and if we can
 assume pthread_mutex_lock() should fail in that mode.  But since I was
 not 100% sure about the comment in the first place, I've not touched
 this code yet.

 > 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;
 > }
 > }}}

 Ah, right.  One possibility is to change it to assert(), just like
 some other similar cases.  But it would probably be better to use
 exceptions when possible, so I introduced a variable to control the
 behavior and set it to an appropriate value depending on the context.

 > 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;
 > }}}

 I didn't mean it's allowed; I meant it may not fail explicitly, like
 with an exception.  But I see the text was actually confusing, and
 even this wrapper method can fail implicitly in non-debug mode.  So I
 revised the documentation just focusing on what would happen in this
 method, removing the reference to pthread_xxx().

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

 Changed/fixed them as suggested.  I personally would like to avoid
 using "destroy" to mean calling the destructor because the former
 could have more general meaning, but admittedly "destructed" as a
 past participle would also look an awkward term.  So I changed it to
 destroyed in the end.

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