BIND 10 #2198: make sure InterprocessSyncLocker is thread safe

[BIND 10 Development]

#2198: make sure InterprocessSyncLocker is thread safe
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20121023
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  Core
  logging                            |  Estimated Difficulty:  5
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 Replying to [comment:14 muks]:
 > Thanks for catching this. I've updated the dependencies and order of
 build. It should build properly now.

 Thanks.

 > > Replying to [comment:12 muks]:
 > If the code were written like this, then true. This is why the API doc
 comment was added asking users of `Mutex` to consider using
 `Mutex::Locker` where applicable. But where it doesn't fit in naturally,
 allowing `lock()` and `unlock()` methods is not bad.

 Well, using lock() and unlock() in a language that has exceptions is very
 risky. I'd very much like to avoid that and disallow that. Jinmei seems to
 agree with the approach (I guess from the condvar discussion in #2332). So
 I'd
 hate to allow it because of it is more slightly more convenient in one
 place.

 I may be too scared of threads, but I know it might be very problematic to
 debug (yes, I did try writing threaded applications before). So I want to
 be on
 the safe side and disallow any kinds of bugs I can.

 > > Also, the tryLock ‒ I guess it is needed somewhere and not provided
 for
 > > completeness? Because I don't see the need for it and it seems to be
 used in
 > > tests only.
 >
 > It's required to test that the other methods work properly for a file
 lock. Also because this is a interprocess synchronization object (not just
 for logger), `tryLock()` is a reasonable operation to provide.

 Well, I have idea of how to do it with a Locker too (throwing exception in
 case
 it isn't possible to lock). But I still don't know if we want such
 operation at
 all, if it is not really used in the real code anywhere.

 > > {{{#!c++
 > > scoped_ptr<Mutex::Locker> locker(new Locker(mutex_));
 > >
 > > if (do_lock(...)) {
 > >     is_locked_ = true;
 > >     locker_ = locker.release();
 > >     return (true);
 > > }
 > >
 > > return (false);
 > > }}}
 >
 > Wouldn't this involve maintaining another `locker_` member variable? Is
 the current code incorrect or complicated to follow? To me the latter code
 and maintaining `locker_` would be more complicated and unnatural in this
 particular case.

 Yes. But then, it means we don't let people directly to the lock() and
 unlock() and get rid of the very awkwardly looking try thing and it would
 IMO be even safer.

 > > This really needs some error checking, EBUSY is not everything the
 pthread_try_lock can return:
 > > {{{#!c++
 > > bool
 > > Mutex::tryLock() {
 > >     assert(impl_ != NULL);
 > >     const int result = pthread_mutex_trylock(&impl_->mutex);
 > >     if (result != 0) {
 > >         return (false);
 > >     }
 > >     ++impl_->locked_count; // Only in debug mode
 > >     return (true);
 > > }
 > > }}}
 >
 > `tryLock()` returns `false` indicating the lock operation failed. Does
 it have to throw as well? I'll add it if you think it's valid.

 Well, but you want to distinguish problems like „The mutex does not exist“
 and
 „The same thread tries to lock it again“ from „The mutex is being locked
 right
 now, try later“. Because the first two are serious programmer errors (the
 second could actually lead to application trying again and again and
 failing
 forever, without providing any notification something is wrong), while the
 last
 is normal operation. It doesn't have to throw, assert like the rest would
 be enough.

 > > What is the point of the exercise with weak pointers, if the code
 still relies on the fact that everything in the map is a valid pointer?
 >
 > As the weak pointer maintains no direct reference, I thought it was more
 understandable code. Or I am not following the question. ^^ :)

 I think that if we want to use them, we want to be prepared for the
 situation
 when we find an invalid weak pointer in the map, just in case something
 forgot
 to remove it. This way, it would throw (I think).

 > > Also, what is the reason behind this?
 > > {{{#!diff
 > > -#include "interprocess_sync_file.h"
 > > +#include <util/interprocess_sync_file.h>
 > > }}}
 >
 > Just for consistency with the rest of the codebase.

 But the second is less corect. There are quote-includes through the code
 as well, if they are from the local directory.

 Anyway, would it be OK if I took the oportunity and wrote a counter-
 proposal for the code as I see it?

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