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

 Hello

 Replying to [comment:11 jinmei]:
 > If you're interested in more detailed discussion, see below.

 OK, I don't want to prolong the discussion then, but I'll just try to
 clarify
 my point of view. Maybe it could get less scary to you.

 > It's an internal detail anyway.  The second mutex is hidden from the
 > public interface.  For a user, condvar still just looks like dealing
 > with a single mutex.

 It stays an internal detail until someone needs to look into the code to
 understand the behavior or to debug the code. The fact that the complexity
 is
 hidden doesn't mean it is not there. I did curse a lot when I was hunting
 a bug
 somewhere around asio ‒ it turned out it was bug in my code, but I needed
 to
 read the code in asio to understand what is wrong about my code.

 > Ah, this approach actually scares me.  Forgetting the friendness in
 > the `CondVar::wait()` internal, I'm scared because this usage example
 > breaks my intuition that `locker` keeps the integrity of holding the
 > lock throughout its lifetime.  But this code actually breaks into it,
 > indirectly via `mutex`, and somehow modifies the state of it and the
 > restores it within wait().  It's fragile to extensions to the `Locker`
 > class, and often leads to strange bugs like "why and how was this
 > condition of `locker` somehow broken"?

 Here we probably disagree in our views. I guess you see the operation as 3
 sequential events:
  * Unlock
  * Wait
  * Lock again

 While I view the wait as single atomic operation „Wait for the state to
 change
 with lock lifted temporarily“. And I believe the atomicity of the
 operation is
 very important there. So I get confused when somebody tries to manipulate
 the
 lock outside of the wait (semi-)manually.

 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.

 > Unfortunately, such a concern tends to be dismissed until we really
 > have a bug due to easily opened holes to break the integrity and spend a
 > large amount of time to debug it, scratching our head.  So I'm
 > pessimistic about this argument being convincing right now.

 Is there a place where it could get inconsistent? The pthread_cond_wait
 will
 always lock the mutex back before terminating. The same thread does not
 run.
 Other threads have no way to know if it was locked or not, so there's no
 observable change.

 OK, there might be a problem with the locked() method. We should make sure
 it
 is correctly handled. But even if not, that one is debug-only and should
 be
 unavailable in release build.

 > I suspect it's a matter of opinion.  You seem to like implicit magic
 > happening within wait().  I'd rather want to make it explicit not to
 > get surprised later.  Whether or not it's a smart pointer, passing
 > `Locker` itself to wait() somehow is crucial in that sense, and I like
 > its clarity - I explicitly know the state of the `Locker` will be
 > changed within wait().

 It may be because I knew pthread conditional variables before and I know
 they
 do the magic inside. So I expect the magic from whatever wrapper I see
 too.

 > Regarding auto_ptr vs shared_ptr, I personally think the property of
 > passing the ownership rarely justifies its pitfalls (and would rather
 > use shared_ptr with a note on the ownership transfer), but if that's
 > the major reason you were against my proposal, I have no problem with
 > using auto_ptr.  But I suspect it's not, so this is a minor point
 > anyway.

 No, I don't like the auto_ptr much too. But I think there may be places
 where
 we need at least that, even when not often.

 I'm not taking the review yet, I still have tickets in my queue. Also, it
 may
 be good if some third party looks at the code besides us two.

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