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

 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.

 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.

 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.

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

 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.

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

 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?

 Also, what is the reason behind this?
 {{{#!diff
 -#include "interprocess_sync_file.h"
 +#include <util/interprocess_sync_file.h>
 }}}

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