BIND 10 #2855: introduce a "zone builder" thread in memory manager

[BIND 10 Development]

#2855: introduce a "zone builder" thread in memory manager
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  shmem         |  reviewing
  manager                            |                    Milestone:
            Keywords:                |  Sprint-20130709
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DNS           |                 CVSS Scoring:
Estimated Difficulty:  4             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Replying to [comment:12 vorner]:
 > Hello
 >
 > About the failing test, the current version of #2854 already fixed
 > that. Can you merge it, please?

 Will do, but just once after #2854 is merged to `master`. I don't want
 to merge from #2854 multiple times. In this particular case, the failing
 testcase doesn't hurt, so we can wait until the final merge time.

 > What does it mean in python to acquire a conditional variable? Does
 > the conditional variable work as mutex as well? If so, is there any
 > reason to have yet another mutex separately? If not, why should this
 > be safe?
 >
 > {{{#!python
 >                 while len(self._command_queue) == 0:
 >                     self._cv.wait()
 > }}}
 >

 A condition object has a lock object associated with it. When we
 `acquire()` on a condtiion (here using the `with` statement), the lock
 is acquired. `wait()` releases the lock when it is called, and when it
 returns, it re-acquires the lock.

 Our extra lock is redundant when we use the queues after acquiring on
 the condition variable. But this is not always so. When a notification
 unblocks `select()` and our callback is called, access to the queues
 will be synchronized only with the lock object. It's very unlikely that
 the builder thread will be unblocked at the same time, but it's better
 to synchronize with a lock object.

 One thing we could do is use the same lock object for the condition
 variable too (to avoid extra acquires in the `MemorySegmentBuilder`
 class). But this is a minor detail.

 > How does the quiting of the thread correlate the fact we don't notify
 > the main thread? How does the main thread know it should join the
 > failed thread? This way, it will probably just sit there and keep
 > filling the command queue, without ever doing anything real.

 Good catch. BTW I assume you mean just the condition where a bad command
 is handled, not the "shutdown" command.

 We now send a 'x' byte on the socket when any reponses are waiting for
 the main thread.

 > What is the advantage of using this instead of just assertEqual?

 I think you missed pasting the place in code that you are referring to
 here.

 > The `test_bad_command` has copy-pasted comments that are not true,
 > they are from the `test_shutdown`. This is both for the part that says
 > it'll put shutdown there and for the one saying the response queue
 > should be empty.

 These comments have been fixed.

 > Regarding the notifications I don't think there's any need for format
 > as such. The socket pair is needed just for one thing ‒ waking up the
 > select. This is quite common pattern and is done like this:
 >  • Write a byte of data to the writing end (non-blocking, if it would
 block, there's already data, so no need to add more).
 >  • Once select is woken up, read everything from there. The data in the
 socket have no meaning, it's just a signal.
 >  • Check the items in the queue and handle them.
 >
 > So, while we don't know the format of data in the response queue, we
 > don't need that one for the implementation of the notifications
 > themselves. It would be similar as with the shutdown command ‒
 > something that would pick up all elements in the queue, run through
 > them and have dummy handler for one.

 Michal, this part is obvious. This is the reason the socketpair exists
 so the loop in the main thread breaks out of its select/poll/whatever.

 The point in my last comment is that we have to test this code by
 sending on this socket which the main thread would handle in its event
 loop. In the callback registered with `watch_fileno()`, it would then
 check the contents of the response queue, and for such code to be
 tested, we would have to decide upon a format for the response queue so
 that `MemorySegmentBuilder` can be modified to handle a test request.

 I have an idea of the response format (it's a tuple and the
 "bad_command" response was formatted as such), but this is best handled
 in #2856 where such notification is actually used.

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