BIND 10 #2836: update in-memory zone loading so it can work with shmem segment

[BIND 10 Development]

#2836: update in-memory zone loading so it can work with shmem segment
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  high          |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         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
-------------------------------------+-------------------------------------

Comment (by jinmei):

 '''other !MemorySegmentGrown cases'''

 I was wrong, and we'd need some more checks for the
 `MemorySegmentGrown` exception.

 - First is `ZoneData::create()`.  Any of `ZoneTree::create`,
   `ZoneTree::insert`, and direct `MemorySegment::allocate()` can
   result in `MemorySegmentGrown`.  In this case it's probably better
   to catch the exception and retry in `ZoneData::create()` (and
   document that this factory doesn't throw `MemorySegmentGrown`)
 - Regarding this, we need some additional tricks for general exception
   safety: `SegmentObjectHolder` cannot simply call destroy() with the
   stored `obj_`, because it may be remapped by the time of the call to
   destroy().
 - `loadZoneDataInternal` also has to be aware that the address
   returned by `holder.get()` might be remapped after the completion of
   load.

 '''others'''

 - as usual, I made some trivial/minor changes.  they are basically
 - editorial/style matters.
 - It's probably worth clarifying the `MemorySegmentGrown`
   consideration for `DomainTree::insert()` in its documentation.  That
   is, this method can throw `MemorySegmentGrown`, and (as already
   documented) as it doesn't provide the strong guarantee, some
   internal nodes may have been added to the tree by the time of
   exception, and they are not removed from the tree due to the
   exception.  But the data integrity shouldn't be broken, so a retry
   (or possibly, though less likely, a few retries) of insert() should
   succeed and provide the originally expected result.

 '''zone_data_updater.cc'''

 - not critical, but is there a reason for extracting `addInternal` as
   a separate method?  It's pretty small and only called from add().
 - add(): I'm not sure if we want to do `set/clearNamedAddress` in
   every call to the method.  This is relatively expensive, yet
   should normally be unnecessary (`MemorySegmentGrown` is a possible
   but very rare event).  I think it's better to set the address in the
   constructor of `ZoneDataUpdater`, and clear it in its destructor.
 - whether it's in add() or the constructor, there's a very slight
   chance that `setNamedAddress()` itself causes address remapping.
   In #2850 we are discussing making it safer/more convenient.
   You may want to use the result (and in any case you may have to
   update this branch if it causes interface change), or otherwise
   you'll have to call `getNamedAddress()` immediately after
   `setNamedAddress()` to make sure the address is really the latest
   one.
 - In #2850 we updated the interface of `getNamedAddress()`  This
   branch will have to be updated due to that, or you may want to
   cherry-pick 68343c845a46a9be3f38c38388ec02a8bb4ef884 (which
   I believe is enough, and it won't be affected in the rest of the
   #2850 review process).

 '''zone_data_updater.h'''

 - Do we really need to expose zone_data_ as a protected member
   variable?  See below for the relevant discussion, but even if we
   really need to allow a derived class to access/modify `ZoneData`
   directly, I'd like to protect integrity as much as possible.  That
   is, unless we need to allow derived classes to modify zone_data_
   (the pointer value itself, not the zone data it points to), I'd only
   allow them to access it via an accessor:
 {{{#!cpp
 protected:
     // zone_data_ itself is still private
     ZoneData* getZoneData() { return (zone_data_); }
 }}}

 '''zone_data_updater_unittest.cc'''

 - memory_segment_mapped will only be available if `--with-shared-memory`
   is specified.  You'll need something like
   10386454c422828a5d46a3b927b1c284e6216372.

 '''zone_data_updater_unittest.cc'''

 - to be very safe it would be better to define a virtual destructor
   for `SegmentCreator`.
 - according to the code it should be possible to use the parameter
   type for ZoneDataUpdaterTest as const:
 {{{#!cpp
 class ZoneDataUpdaterTest : public ::testing::TestWithParam<const
 SegmentCreator*> {
 }}}
   Although it somehow didn't compile in my environment.
 - Likewise `test_segment_creator` etc should be able to be const
 {{{#!cpp
 TestSegmentCreator test_segment_creator;
 }}}
   and we shouldn't need cast
 {{{#!cpp
 INSTANTIATE_TEST_CASE_P(TestSegment, ZoneDataUpdaterTest,
                         ::testing::Values(static_cast<SegmentCreator*>(
                             &test_segment_creator)));
 }}}
   But these don't seem to work in my environment either.
 - `TestZoneDataUpdater`: if I understand the tests correctly, I
   believe we can do the tests without this: we'd first create
   `ZoneData` in `ZoneDataUpdaterTest`, set it in the memory segment
   with a name, and let the tests get the zone data when necessary
   via (maybe a wrapper of) `getNamedAddress()`.  This will certainly
   make the test setup more complicated, but I personally would like to
   pay this cost if the alternative is to provide a mutable accessor
   which wouldn't be necessary otherwise.  With this approach, we
   shouldn't even need the `getZoneData()` protected method mentioned
   above, let alone making `zone_data_` itself protected.
 - `ZoneDataUpdaterTest`: is there a reason `mem_sgmt_` is a shared
   pointer, not scoped?
 - `ZoneDataUpdaterTest` constructor: technically this doesn't seem to
   be exception safe:
 {{{#!cpp
         updater_(new
                  TestZoneDataUpdater(*mem_sgmt_, zclass_, zname_,
                                      *ZoneData::create(*mem_sgmt_,
 zname_)))
 }}}
   if constructing `TestZoneDataUpdater` throws.  same for
 `clearZoneData()`.
 - `ZoneDataUpdaterTest` destructor: as far as I can see `updater_`
   should be always non NULL here:
 {{{#!cpp
         if (updater_) {
             ZoneData::destroy(*mem_sgmt_, updater_->getZoneData(),
 zclass_);
         }
 }}}
 - `MappedSegmentCreator::cleanup`:
 `boost::interprocess::file_mapping::remove`
   is probably better than unlink.  It would be more portable (at least
   to the extent of the theory that Boost is expected to be portable),
   and it also seems to have some consideration on mmap specifics (and
   so safer).
 - `manySmallRRsets`: is this guaranteed, or is this just a guess?  I
   thought we cannot completely predict the grow timing as it highly
   depends on the boost implementation internal.  If this is a guess,
   I'd clarify the wording accordingly as it's not clear to me.
 {{{#!cpp
 // Some of the grows will happen inserting the RRSIG, some with the TXT.
 }}}

-- 
Ticket URL: <https://bind10.isc.org/ticket/2836#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development