BIND 10 #2205: introduce a "data source configurator" thread in auth

[BIND 10 Development]

#2205: introduce a "data source configurator" thread in auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121023
  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 jelte):

 * owner:  jelte => jinmei


Comment:

 (i have no strong opinion on whether to use functors, so I'm fine with
 this approach for now)

 The DataSrcClientsBuilderTest.start test segfaults for me (in
 shutdownCheck); i fear the std::list object is cleared when the clientsmgr
 is destroyed (so the pointer is still there but size() runs into trouble),
 if this works for you i do wonder what valgrind thinks about it :)

 Looks like this is already mentioned in the destructor comments, where it
 is noted that the thread must have been stopped before destroying the
 base.

 Not sure if there is an easy way to fix this though (short of making the
 member in base a pointer or a passed reference). Perhaps read the value in
 wait()? (i.e. after sendCommand but before actual desctruction). We can't
 check whether it doesn't send more than 1 message though, then.

 {{{
 Program received signal SIGSEGV, Segmentation fault.
 __distance<std::_List_const_iterator<std::pair<isc::auth::datasrc_clientmgr_internal::CommandID,
 boost::shared_ptr<isc::data::Element const> > > > (
     __last=<optimized out>, __first=<optimized out>)
     at /usr/include/c++/4.6/bits/stl_iterator_base_funcs.h:82
 82                ++__first;
 (gdb) where
 #0
 __distance<std::_List_const_iterator<std::pair<isc::auth::datasrc_clientmgr_internal::CommandID,
 boost::shared_ptr<isc::data::Element const> > > > (
     __last=<optimized out>, __first=<optimized out>)
     at /usr/include/c++/4.6/bits/stl_iterator_base_funcs.h:82
 #1
 distance<std::_List_const_iterator<std::pair<isc::auth::datasrc_clientmgr_internal::CommandID,
 boost::shared_ptr<isc::data::Element const> > > > (
     __last=<optimized out>, __first=<optimized out>)
     at /usr/include/c++/4.6/bits/stl_iterator_base_funcs.h:117
 #2  size (this=<optimized out>) at
 /usr/include/c++/4.6/bits/stl_list.h:846
 #3  shutdownCheck () at datasrc_clients_mgr_unittest.cc:35
 #4  (anonymous namespace)::DataSrcClientsMgrTest_start_Test::TestBody (
     this=<optimized out>) at datasrc_clients_mgr_unittest.cc:59
 #5  0x00000000004f07ad in void
 testing::internal::HandleExceptionsInMethodIfSupported<testing::Test,
 void>(testing::Test*, void (testing::Test::*)(), char const*) ()
 #6  0x00000000004e92a7 in testing::Test::Run() ()
 #7  0x00000000004e937d in testing::TestInfo::Run() ()
 #8  0x00000000004e94c7 in testing::TestCase::Run() ()
 #9  0x00000000004e9757 in testing::internal::UnitTestImpl::RunAllTests()
 ()
 #10 0x00000000004f032d in bool
 testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,
 bool>(testing::internal::UnitTestImpl*, bool
 (testing::internal::UnitTestImpl::*)(), char const*) ()
 #11 0x00000000004e894b in testing::UnitTest::Run() ()
 }}}


 And some smaller points on the code itself:

 datasrc_clients_mgr.h:

 - DataSrcClientsMgrBase destructor comment: "this could time" I think you
 meant "this could take a long time"

 - the DataSrcClientsMgrBase destructor logs on normal exception, but not
 on the catchall. Those shouldn't happen, but should we not log there too?

 - #ifdef notyet   A fine approach to 'temporary dead code with plan to
 fix', should we standardize on this method? (up to now i have tended to
 use '#if 0', and that is what i'd grep for if i was looking for dead code)

 {{{
 /// This class is a server of \c DataSrcClientsMgr.  Except for tests,
 /// applications should not directly access to this class.
 }}}
 I don't really understand this comment (and the second line has an error,
 it misses 'have')


 auth_messages.mes: you forgot to reorder it :)


 test_datasrc_clients_mgr.h: I know these shouldn't happen anyway, and are
 in tests, so this is a really trivial point, but the two isc_throw strings
 could use a slightly better text

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