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

 Replying to [comment:17 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?

 I mean the suggestion from comment 14:
 {{{
 But if it is more explicit to you, we could pass the Locker as the
 parameter of
 wait instead of Mutex itself (and get locker.mutex_->impl.mutex from it
 internally). That is uglier on the inside, but you explicitly see the
 locker
 being passed and you don't need to assert it is locked, because it just
 must
 be.
 }}}

 I was asking if it helped if the call looked like:
 {{{#!c++
 condvar.wait(locker);
 }}}

 instead of

 {{{#!c++
 condvar.wait(mutex);
 }}}

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

 I don't know why we didn't use it, I think some of the build bots didn't
 have
 this macro. But we may try and see if one of them complains.

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

 Well, if we acquired the lock, the mutex must have not been locked before
 (because we don't have the recursive mutexes now). The mutex would block
 until
 unlocked first. And therefore the count must be 0. I see only one
 possibility
 when it could be non-zero ‒ if we counted the locks/unlock badly. So, I
 think
 this only checks our debug aid kit works, not if the code itself does.

 Or, do I miss something?

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