introduce a "data source configurator" thread in auth

[jinmei]

A subtask of #2201.

First, fully consider how to test these without creating threads.

In this task, we let auth spawn a separate thread with a
synchronization mechanism with the main auth thread. The main and
configurator thread work as the producer and the consumer: the main
thread (producer) passes general form of command in the form of
ConstElementPtr and the configurator thread (consumer) accepts each
command and execute it.

Choose an appropriate synchronization mechanism for the concept and
implement it. One way is to let both threads share
vector<ConstElementPtr> with a conditional variable to get access to
it.

In this task, we only support "shutdown" command. On auth's shutdown,
the main thread sends this command to configurator, and waits for it
to die by join(). The configurator simply exits if it gets this
command.

Same open question as in #2202 applies. We use the consistent policy
on this.

This ticket depends on #2332 if we use condition variables.

[vorner]

Maybe it's a bit of haskellism on my side, but whenever I do something like this, the easiest (and most generic) approach is creating a work thread with a queue of functions (or functors) ‒ something like a wrapper around a thread, conditional variable and vector<boost::function<void()> >.

It is simple, generic and the code that needs to be backgrounded (like the loading we plan) doesn't have to be split across multiple places ‒ the code that needs something done in the background contains the functor as well.

[jinmei]

Replying to vorner:

Maybe it's a bit of haskellism on my side, but whenever I do something like this, the easiest (and most generic) approach is creating a work thread with a queue of functions (or functors) ‒ something like a wrapper around a thread, conditional variable and vector<boost::function<void()> >.

It is simple, generic and the code that needs to be backgrounded (like the loading we plan) doesn't have to be split across multiple places ‒ the code that needs something done in the background contains the functor as well.

I'm not sure if I fully understand above. Can you provide some
sample code snippet to illustrate the idea more specifically?

In any case, specific ways to implement it is up to the developer
(and the reviewer, and if it's controversial on review we can discuss
it project wide). The approach mentioned in the description is just
an example to give a better idea of what should be done, and doesn't
have to be adopted literally.

[vorner]

Hello

Replying to jinmei:

I'm not sure if I fully understand above. Can you provide some
sample code snippet to illustrate the idea more specifically?

OK. I'll skip details like condition variables, mutexes, etc. Let's say we have
a SynchronizedQueue container that allows to put objects in and take them out
in separate threads, the taking blocks if there's no object.

class WorkThread {
private:
        SynchronizedQueue<boost::function<void ()> > queue_;
public:
        // Called from the "main" thread. Never blocks.
        void asyncWork(boost::function<void ()> work) {
                queue_.push_back(work);
        }
        // Running "for ever" in the work/background thread.
        void run() {
                for (;;) {
                        boost::function<void ()> work = queue_.pop_front();
                        work();
                }
        }
};

Then, if we want to handle configuration update, we do something like this:

// This one runs a long time, should be run in background
void handleDatasrcConfig(const isc::data::ConstElementPtr& config) {
        ...
}

void configHandler(const isc::data::ConstElementPtr& config) {
        ...
        workThread.asyncWork(boost::bind(handleDatasrcConfig, config->get("data_sources")));
        ...
}

I hope it explains the idea.

In any case, specific ways to implement it is up to the developer
(and the reviewer, and if it's controversial on review we can discuss
it project wide). The approach mentioned in the description is just
an example to give a better idea of what should be done, and doesn't
have to be adopted literally.

Indeed. I just wanted to share the idea if the implementor finds it usable,
because it seems to me the technique is elegant, but mostly unknown in the
procedural world.

[jinmei]

trac2205 is ready for review.

It depends on #2332, which is also under review, but based on the
review discussion, I believe the end result of #2332 doesn't affect
the fundamental design of this branch, and so review of this branch
can be started now.

The diff to be reviewed is the result of 'git diff 4629279'. The
commits before this is to incorporate a snapshot version of #2332 and
should be ignored for this review.

I first suggest reading datasrc_clients_mgr.h and its comments. It
describes general design in some detail, and also has most of the
actually implementation.

There's one unrelated cleanup: b4ec1b3. It's totally unrelated but
something I just happened to notice and could be easily fixed. So I
did it here.

Regarding the design: to be honest I forgot the intermediate comment
by Michal at http://bind10.isc.org/ticket/2205#comment:5 and have
completed the current implementation; now that I have a specific
design and implementation maybe I'm already biased, but after thinking
about it for a while I decided to keep the current design. I see
advantages of the suggested way: it can be more generic, but at the
same time we need to define specific work functors in the main thread
side, which means to test them we need to expose them more visibly. I
wanted to keep these details as much as possible, and preferred the
current way where detailed work logic is hidden in an essentially
private class. But in any case, we don't have much real work logic
yet, so if someone wants to revise this part with the functor approach
in #2210 or #2212, I don't oppose to that just because the initial
implementation doesn't adopt it.

[jelte]

(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

[jinmei]

Thanks for the review.

Replying to jelte:

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

Ah, yes, you're right. It was obviously wrong.

I couldn't come up with a very clean solution either, and tried to
address it at commit c2608e7. I believe it should at least solve the
crash issue.

datasrc_clients_mgr.h:

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

Ah, right, fixed.

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

I thought it might rather make sense to eliminate the possibility of
having another exception, but since the risk is already there, I'm
okay with logging it too. Added.

• #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)

Standardizing it sounds like a good idea. I'd suggesting something
unique to us in that case, e.g., ifdef BIND10_TODO. But in any case
it would be beyond the scope of this ticket (and in this particular
case this ifdef should really be removed very soon anyway). So I
didn't touch it.

/// 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')

Okay, I've updated the whole paragraph.

auth_messages.mes: you forgot to reorder it :)

Ah, right, thanks. I intended to reorder it but forgot.

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

Okay, I tried to make it better.

[jelte]

Looks good; I have pushed one final change (initialize signal_count in the TestCondVar? constructor)

with that I think it is ready for merge.

[jinmei]

Replying to jelte:

Looks good; I have pushed one final change (initialize signal_count in the TestCondVar? constructor)

with that I think it is ready for merge.

Thanks (for the check and fix), merge done, closing.