BIND 10 #2202: introduce a lock for the data source client in auth

[BIND 10 Development]

#2202: introduce a lock for the data source client in auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121009
  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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 Replying to [comment:21 jinmei]:
 > > This goes against the way I think about the impl class. I consider it
 being a
 > > data-only, so such complicated initialization doesn't belong there.
 Also,
 > > having the initialization there and everything else outside seems
 strange and
 > > inconsistent. The overhead for the `auto_ptr` should be small (and
 it's during
 > > initialization only).
 >
 > Hmm, at least that's not against the sense of pimpl at all.  My
 > understanding of the point of pimpl is to hide all detailed
 > definitions from the public interface to maximize binary
 > compatibility, minimize unnecessary build overhead (as header files
 > contain minimal info/code), and even more flexibility of changing
 > internal details without worrying about breaking anything.

 I wasn't talking about a pimpl in general, but against this exact impl
 class.

 > The 1.67% drop in the average case is probably acceptable, but frankly
 > I'd be concerned about the 5%-ish worse case overhead, especially
 > because the lock should be unnecessary in the vast majority of cases.
 > In future versions we should probably think about making the
 > performance sensitive code lock-free, but for now, I think we should
 > accept the overhead and complete the feature.

 I hope the non-debug build (there's a ticket for it somewhere) should
 improve
 it significantly.

 > - "or even destructors" is now incorrect (see the next bullet)
 > {{{#!cpp
 > /// constructors or even destructors in this class can throw. Allocation
 errors
 > }}}
 > - Mutex destructor: it's now exception free.  Same for ~Locker().

 Actually, the ~Locker can still throw. Should I keep the comment or make
 sure
 it aborts too?

 > - I'd avoid hardcoded constant:
 > {{{#!cpp
 >         waiting_(2),
 > }}}

 I don't know. It would look harder to read to me if it was through a
 constant
 somewhere, like waiting_(initial_number_of_events). I added a comment
 there.

 > - `Thread` class constructor: did you intentionally use "zero" in '0K'?

 Yes, of course O:-).

 > - `Thread::wait()`: mostly a theoretical concern, but since this
 >   method can throw, it can make the object a strange state.  For
 >   example, consider a very rare case of new'ing `UncaughtException`
 >   throws.  Then even if pthread_join() was called impl_ will be still
 >   non NULL.  But I have no good idea to address this concern in a
 >   concise and clean way.  Maybe it should be very minor that we can
 >   ignore it.  In that case I'd add some note about it at least.

 I don't know if this covers completely everything, but I don't think there
 could be other place this could go wrong now.

 > - `DISABLED_destroyLocked`: forgetting the FIXME issue, this test
 >   isn't correct any more anyway because ~Mutex() now doesn't throw.
 >   If we want to test the condition, we'd need to change it to a death
 >   test (and I'm not sure if it would work as a workaround for the
 >   FIXME issue.  Maybe you can try it).

 Hmm, right. It helped with the FIXME.

 > - swarm test: I was impressed your imagination:-) but
 >   performStrangeOperation seems to be way too much complicated for the
 >   purpose.  After reading the comment and code 10 minutes, I found I
 >   still don't fully understand the code, much less whether it does
 >   what the document says.  I could spend for a few more hours to fully
 >   understand it with confidence, but before that please let me ask: do
 >   we really need this level of complexity?  I suspect, in practice,
 >   a much simpler test case is sufficient.

 When writing it, I spend quite some time trying to come up with something
 that:
  * The compiler shouldn't be able to optimize it to something atomic (like
 an
    increment on one integer which is atomic on many architectures).
  * Would keep some kind of invariant on the data when doing it correctly.
  * Would break with a high chance when accessing it in parallel.
  * Would not mess the memory completely, so we can safely detect something
 is
    wrong but not crash.

 Unfortunately, after few iterations why my current solution would not
 work, I
 came only to this. If you have something simpler in mind, it would be
 welcome.
 It's not called „strange“ operation for no reason currently :-|.

 I tried adding some more comments into the code, if it helps a bit.

 > - swarm test: independent from the test case details, when we test
 >   locks, there's a risk of having deadlock due to a bug.  That would
 >   be bad for auto-running tests.  Should we add some safety net like a
 >   deadline timer (if it expires it terminates the entire process
 >   unconditionally).

 I don't think it can happen, as there's only one mutex. But I added the
 timeout anyway.

 >   although I admit there are other places in our code that implicitly
 >   relies on C99.  I' also not sure if it's valid even without C99 when
 >   length is a const variable, but the safest way would be to use
 >   std::vector.

 I used vector directly. It should be safe this way.

 > {{{#!cpp
 > void
 > doSomething(int* x) {
 >     *x = 42;                  // may cause race, but it doesn't matter
 > }
 >
 > TEST(ThreadTest, detached) {
 >     int x;
 >     for (size_t i = 0; i < detached_iterations; ++i) {
 >         Thread thread(boost::bind(&doSomething, &x));
 >     }
 > }
 > }}}

 This would be in fact very dangerous. The TEST() method can terminate
 sooner
 than the test, and the test could overwrite part of the stack in some
 other
 function. I ignore the parameter for now.

 > - comment style is different than the expected one per guideline
 > {{{#!cpp
 >     /**
 >      * \brief Return a mutex for the client lists.
 >      *
 > }}}
 >   etc.

 But the /// ones break the \code snippets :-(. How is it done, then?

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