BIND 10 #2198: make sure InterprocessSyncLocker is thread safe

[BIND 10 Development]

#2198: make sure InterprocessSyncLocker is thread safe
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  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 muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:13 vorner]:
 > Hello
 >
 > First of all, it doesn't compile for me on a clean build ‒ the
 directories need
 > some reordering. Please, try a clean build and see.

 Thanks for catching this. I've updated the dependencies and order of
 build. It should build properly now.

 > Replying to [comment:12 muks]:
 > > `Mutex::tryLock()` has been added (a requirement for our `tryLock()`)
 and `Mutex::lock()` and `Mutex::unlock()` were made public.
 `Mutex::tryLock()` has to be public anyway. The way we use a
 `Mutex::Locker`, it doesn't add benefit to dynamically allocate it (so it
 lives when it runs out of scope) when we can call the same methods on
 `Mutex` directly and the lock goes when it is destroyed.
 >
 > I'm not really happy about that. With this, we'll see stuff like this to
 appear in some time:
 >
 > {{{#!c++
 > Locker locker(mutex);
 > ...
 > ...
 > // Unlock for a short while
 > mutex.unlock();
 > doSomething();
 > mutex.lock();
 > ...
 > ...
 > }}}
 >
 > This would be very bad if doSomething() threw.

 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.


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


 > Also, with the dynamic allocation and use of some kind of scoped_ptr,
 you could get rid of the try-catch block here:
 > {{{#!c++
 >     // First grab the thread lock...
 >     mutex_->lock();
 >
 >     // ... then the file lock.
 >     try {
 >         if (do_lock(F_SETLKW, F_WRLCK)) {
 >             is_locked_ = true;
 >             return (true);
 >         }
 >     } catch (...) {
 >         mutex_->unlock();
 >         throw;
 >     }
 >
 >     mutex_->unlock();
 > }}}
 >
 > {{{#!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.

 > Also, what is the difference with lock and tryLock? While the use of
 mutex is slightly different, but the call to do_lock looks exactly the
 same to me.

 `F_SETLKW` vs. `F_SETLK`.

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

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

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

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