BIND 10 #2861: synchronization between auth main thread and datasrc builder

[BIND 10 Development]

#2861: synchronization between auth main thread and datasrc builder
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  pselkirk
            Priority:  medium        |                       Status:
           Component:  b10-auth      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130723
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  4             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => pselkirk


Comment:

 Hello

 Replying to [comment:13 pselkirk]:
 > First a nit: When you changed `Command` from pair to struct, you removed
 the documentation about the members:
 > {{{
 > -/// The first element of the pair is the command ID, and the second
 element
 > -/// is its argument.  If the command doesn't take an argument it should
 be
 > -/// a null pointer.
 > }}}
 >
 > It's a minor point, but I like having documentation, especially when
 arguments are optional.

 I added the documentation (I thought the original was mapping between the
 „first“ and „second“ names and the actual roles, and that the current
 names are self-explanatory).

 But, I don't see any optional arguments here.

 > In `run()`, I'm a bit more concerned about the untested code that
 terminates on wake error. When I added a `close(write_end)` to the test
 code to trigger this, I got
 > {{{
 > terminate called without an active exception
 > make[5]: *** [check-local] Aborted (core dumped)
 > }}}
 >
 > It seems that it would be better to throw an exception, which would be
 immediately caught and logged by the catch block. The program would still
 terminate, but with something like
 > {{{
 > terminate called after throwing an instance of 'isc::Unexpected'
 >   what():  failed to wake up main thread
 > }}}

 Would it? Generally, the `stderr` of the program is not shown to anybody
 anyway and throwing and catching it in the same function seems like badly
 masked `goto`, confusing the flow.

 I believe the more important thing is logging, than `stderr` output. My
 version does that, and with more concrete log message than the proposed
 one. It's just that the logging is turned off during tests by default.

 However, I'd be interested in what exactly you did to trigger it. It might
 be useful to build a test case on top of that.

 > Finally, I understand the purpose of `FDGuard`, but I don't understand
 why you don't just put the `close()` calls in the `DataSrcClientsMgrBase`
 destructor, or in `cleanup()`. Why do you need an extra object for this?

 I might be wrong here. But AFAIK if an exception happens when calling one
 of the initializers in the constructor, the only the destructors for the
 already initialized members are called (in reverse order), but not the
 main destructor of the class. So, if `builder_thread_`, for example,
 threw, the `close()` wouldn't get called at all and both the file
 descriptors would leak. As the guard is initialized before the file
 descriptors, its destructor gets called even when the whole constructor
 did not run yet.

 I did not include this explanation in a comment in the code, because I
 believe there are many cases in our code like this. But if you think this
 case is more complex than usual, I can try writing it down there too.

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