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

 In conclusion: I adopted your proposed approach and picked up the
 ticket.

 If you're interested in more detailed discussion, see below.

 Replying to [comment:9 vorner]:

 > > I didn't think it possible to resolve this situation through making
 > > `CondVar` a friend of `Mutex`, hence the introduction of pointer to
 > > `Locker`.  With that introduction `CondVar` simply didn't have to be a
 > > friend of `Mutex`, and that's why I didn't make it so.
 > >
 > > I see the concerns you had in this proposal, and I don't think this
 > > one is the best one in terms of complexity and understandability.  If
 > > there's a design to make it possible in a safe and more
 > > understandable, I'm happy to use it.  Do you have a concrete proposal?
 >
 > Well, for one, I always used the condvars with single mutex only, not
 two. And
 > I think it is easier to grasp. I don't know why boost used two, but I
 don't see
 > a reason for us. I guess they didn't document a reason for doing so in
 their
 > case and consider boost many things, but simple is really not one of
 them.

 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.  I admit, though, the entire implementation will
 look more complicated if you are also the implementor of the internal.
 I guess the main reason for the boost approach is to allow various
 types of mutex types, and not to break into details of the mutex
 details.  The latter is related to our case.  More to follow.

 > And if we made CondVar friend of Mutex and put CondVar into the same C++
 file
 > (so it would know the Impl of Mutex), we could write directly something
 like
 > this:
 > {{{#!c++
 > void
 > CondVar::wait(Mutex& mutex) {
 >     assert(mutex.locked());
 >     pthread_cond_wait(impl_->condvar, mutex.impl_->mutex);
 > }
 > }}}
 >
 > It does indeed make CondVar and Mutex very close friends, but they are
 indeed
 > quite related. But then we can rewrite your example as:
 > {{{#!c++
 > void
 > run() {
 >     // some kind of mutex to protect the command queue, shared with the
 main
 >     // thread.
 >     Mutex mutex;
 >     CondVar condvar;            // shared with the main thread
 >
 >     while (true) {
 >       Command command;
 >       {
 >             Mutex::Locker locker(mutex);
 >             while (queue.empty()) {
 >                 condvar.wait(mutex); // Here we pass locked mutex. As
 pthread condvar wants.
 >             }
 >             // at this point mutex is locked and queue is not empty
 >             command = queue.pop();
 >       } // release the mutex as the following would be time consuming
 >
 >         // do something based on command: reload a zone, reconfigure
 entire
 >         // data source clients, or shutdown the thread, etc.
 >     }
 > }
 > }}}

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

 And then the wait() implementation exploiting the friendness.  It
 breaks the general assumption that an application can acquire a lock
 for a `Mutex` object only via `Locker`, and in a very fierce way: it
 doesn't even use the lock()/unlock() methods, but intrudes into the
 deepest part of the `Mutex` internal and modify it directly.
 Immediately after introducing this, our code will have a ticking bomb
 which explodes when we first encounter a bug due to a strange
 situation when a mutex is magically acquired (or released).

 BTW, the essential point is not the use of friend; I'm not a member of
 the anti-friend religion.  The point is an easy trade of safety and
 robustness for seeming convenience.  So, even if it's implemented
 without a friend but with many mutable methods, the same point will
 apply (although friend is generally more dangerous of its excessive
 power).

 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.

 And, to be fair, I understand this is also a tradeoff issue.
 There can be cases where convenience outweighs other risks of breaking
 class integrity, and, as any other tradeoff issues, different people
 have different opinions on the reasonable balance.  Further, in this
 particular case, I have to admit my alternative suggestion wouldn't
 really be a good one.  At least it will make the initial
 implementation more complicated, and the use of multiple mutexes would
 more easily cause synchronization related bugs.

 Also, since this wouldn't be part of public API (for general use
 people should use things like boost::thread), so the choice of the
 initial design won't be that critical; while perhaps costly, we can
 change the interface and implementation later if we really find
 problems.

 So, I withdraw my proposal, and will implement it in the way you
 suggested above.

 > > And, one more thing: I also tend to agree that using `Locker` through
 > > a pointer could be considered a kind of abuse, but if so, I think the
 > > documentation should state that more clearly.  It currently reads:
 > >
 > > {{{#!cpp
 > >     /// If you create the locker on the stack or using some other
 "garbage
 > >     /// collecting" mechanism (auto_ptr, for example), it ensures
 exception
 > >     /// safety with regards to the mutex - it'll get released on the
 exit
 > >     /// of function no matter by what means.
 > > }}}
 > >
 > > This looks like it's actually expected to be used through auto_ptr,
 > > even though it's not explicitly encouraged.  If such usage is rather
 > > discouraged IMO we should say something like "even if something like
 > > this is possible, it's generally discouraged".
 >
 > Well, auto_ptr still seems better than shared_ptr, because you have just
 one
 > instance. When you do a release on shared_ptr, you never know if someone
 else
 > is still holding it or not. Anyway, I'm not against discouraging it
 more. I'm
 > OK with discussing using it when the situation requires, but I'd hate to
 > _require_ it through one of our interfaces.

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

 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.

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