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):

 Replying to [comment:19 vorner]:

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

 To me, it doesn't seem to help much, so I'd keep the current
 interface.

 This usage seems to be counter intuitive for the basic nature
 of `Locker`, which would be normally instantiated on the stack for a
 small scope and would just stay there, keeping the state at the time
 of instantiation.  In my previous usage with passing a pointer and
 getting a new pointer on return, I tried to clarify that the ownership
 is completely transferred to wait() and the caller cannot assume the
 passed locker object is valid any more.  If we don't go that far, I
 don't see much benefit in being counter intuitive, and would rather
 give the illusion as if `locker` were intact throughout the scope.

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

 Based on your comment I guess we are talking about the same thing.
 So, what would you want it to be?  changing it to an assertion?
 Or add a comment like this?

 {{{#!cpp
     // This check should fail only in the debug mode and when the
 application
     // tries to acquire the lock recursively.  Depending on how we realize
     // the non debug mode this might be just a dead check.
     if (impl_->locked_count != 0) {
         isc_throw(isc::InvalidOperation, "Lock attempt for locked mutex");
     }
 }}}

 Or even just remove the entire if block?  I'm fine with any of them
 (or if it's something else please specify it), although I less prefer
 the last option - when it comes to thread related code, it would be
 better to be bit more paranoid than necessary in case we miss
 something or introduce future regressions.

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