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-20120918
                   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:6 vorner]:
 > Hello
 >
 > I have several concerns about the branch.
 >
 > First, the branch #2202 introduces a wrapped lock (and a thread, but it
 shouldn't be needed here). I think we should not be dealing directly with
 pthreads through the code, both because of exception safety and error
 handling and because of portability. Is there any reason why you didn't
 use the lock from there?

 I didn't know that there's a new lock class introduced in the #2202
 branch. :) That is the first answer for why I didn't use that lock. For
 the RAII style lock, #2202's locker seems relevant. Ignoring that the
 branch is still in review, we still can't use this implementation here as
 the interface provided by that lock class is insufficient. It's not
 possible to unlock the mutex before its destruction (i.e., before that
 basic block is exited). I have not read more into #2202.

 Use of pthreads in this file should be fine as it is a lock class
 implementation. We use other POSIX specific bits in this class such as
 `fcntl()`. The locker in #2202 also uses pthreads.

 > Furthermore, the code seems to be doing no error checking whatsoever.
 Most of the pthread functions can return error codes and they should be
 handled. Also, the new or insertion to the map can throw. If that happens,
 the mutex stays locked, causing a deadlock the next time anyone tries to
 acquire it.

 I agree with the use of a RAII-style lock. A simple wrapper around a
 pthread mutex has been introduced. All pthread calls are now checked. Even
 then, I don't suppose our code can catch such exceptions and continue in
 case the map throws in this codepath.

 > Also, the `lock`, `tryLock` and `unlock` perform find on the map without
 locking the lock. That is problematic ‒ what if someone modifies it while
 they perform the find?

 Good catch. :) The `find()` is now protected by a lock.

 >
 > Is there really need for the complicated thing with the map? Will there
 ever be multiple locks with the same task name in the same process?

 This is what `InterprocessSyncFile`'s interface for the file locking
 provides (among multiple processes anyway) and we keep that semantic. It's
 not unlikely for multiple logging threads to exist, all of which use the
 "logger" name.

 >
 > And, last, how often is the lock on the thing called? Are there some
 performance implications?

 Our typical use case is the logger. Logging calls happen once every few
 hundred ms on average, so it doesn't have a big impact. The existing
 (thread-unsafe) code uses file range locks which are slower.

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