[2836] Set the segment holder separately

Only allocate the memory for segment holder in the constructor, set it
in separate method. The allocation might throw.

src/lib/datasrc/memory/segment_object_holder.h

@@ -42,19 +42,30 @@ getNextHolderName();
 template <typename T, typename ARG_T>
 class SegmentObjectHolder {
 public:
-    SegmentObjectHolder(util::MemorySegment& mem_sgmt, T* obj, ARG_T arg) :
+    SegmentObjectHolder(util::MemorySegment& mem_sgmt, ARG_T arg) :
         mem_sgmt_(mem_sgmt), arg_(arg),
         holder_name_(getNextHolderName()), holding_(true)
     {
-        mem_sgmt_.setNamedAddress(holder_name_.c_str(), obj);
+        if (mem_sgmt_.setNamedAddress(holder_name_.c_str(), NULL)) {
+            isc_throw(isc::util::MemorySegmentGrown,
+                      "Segment grown when allocating holder");
+        }
     }
     ~SegmentObjectHolder() {
         if (holding_) {
             // Use release, as it removes the stored address from segment
             T* obj = release();
-            T::destroy(mem_sgmt_, obj, arg_);
+            if (obj) { // May be NULL if set wasn't called
+                T::destroy(mem_sgmt_, obj, arg_);
+            }
         }
     }
+    void set(T* obj) {
+        const bool grown = mem_sgmt_.setNamedAddress(holder_name_.c_str(),
+                                                     obj);
+        // We reserve the space in the constructor, should not grow now
+        assert(!grown);
+    }
     T* get() {
         if (holding_) {
             const util::MemorySegment::NamedAddressResult result =

src/lib/datasrc/memory/zone_data.cc

@@ -91,8 +91,8 @@ NSEC3Data::create(util::MemorySegment& mem_sgmt,
     // (with an assertion check for that).
     typedef boost::function<void(RdataSet*)> RdataSetDeleterType;
     detail::SegmentObjectHolder<ZoneTree, RdataSetDeleterType> holder(
-        mem_sgmt, ZoneTree::create(mem_sgmt, true),
-        boost::bind(nullDeleter, _1));
+        mem_sgmt, boost::bind(nullDeleter, _1));
+    holder.set(ZoneTree::create(mem_sgmt, true));
 
     ZoneTree* tree = holder.get();
     const ZoneTree::Result result =
@@ -165,8 +165,8 @@ ZoneData::create(util::MemorySegment& mem_sgmt, const Name& zone_origin) {
     // NSEC3Data::create().
     typedef boost::function<void(RdataSet*)> RdataSetDeleterType;
     detail::SegmentObjectHolder<ZoneTree, RdataSetDeleterType> holder(
-        mem_sgmt, ZoneTree::create(mem_sgmt, true),
-        boost::bind(nullDeleter, _1));
+        mem_sgmt, boost::bind(nullDeleter, _1));
+    holder.set(ZoneTree::create(mem_sgmt, true));
 
     ZoneTree* tree = holder.get();
     ZoneNode* origin_node = NULL;

src/lib/datasrc/memory/zone_data_loader.cc

@@ -183,7 +183,8 @@ loadZoneDataInternal(util::MemorySegment& mem_sgmt,
                      boost::function<void(LoadCallback)> rrset_installer)
 {
     SegmentObjectHolder<ZoneData, RRClass> holder(
-        mem_sgmt, ZoneData::create(mem_sgmt, zone_name), rrclass);
+        mem_sgmt, rrclass);
+    holder.set(ZoneData::create(mem_sgmt, zone_name));
 
     ZoneDataLoader loader(mem_sgmt, rrclass, zone_name, *holder.get());
     rrset_installer(boost::bind(&ZoneDataLoader::addFromLoad, &loader, _1));

src/lib/datasrc/memory/zone_table.cc

@@ -50,8 +50,8 @@ typedef boost::function<void(ZoneData*)> ZoneDataDeleterType;
 ZoneTable*
 ZoneTable::create(util::MemorySegment& mem_sgmt, const RRClass& zone_class) {
     SegmentObjectHolder<ZoneTableTree, ZoneDataDeleterType> holder(
-        mem_sgmt, ZoneTableTree::create(mem_sgmt),
-        boost::bind(deleteZoneData, &mem_sgmt, _1, zone_class));
+        mem_sgmt, boost::bind(deleteZoneData, &mem_sgmt, _1, zone_class));
+    holder.set(ZoneTableTree::create(mem_sgmt));
     void* p = mem_sgmt.allocate(sizeof(ZoneTable));
     ZoneTable* zone_table = new(p) ZoneTable(zone_class, holder.get());
     holder.release();
@@ -77,8 +77,8 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class,
     if (content == NULL) {
         isc_throw(isc::BadValue, "Zone content must not be NULL");
     }
-    SegmentObjectHolder<ZoneData, RRClass> holder(mem_sgmt, content,
-                                                  zone_class);
+    SegmentObjectHolder<ZoneData, RRClass> holder(mem_sgmt, zone_class);
+    holder.set(content);
     // Get the node where we put the zone
     ZoneTableNode* node(NULL);
     switch (zones_->insert(mem_sgmt, zone_name, &node)) {

src/lib/datasrc/tests/memory/rdataset_unittest.cc

@@ -193,18 +193,18 @@ TEST_F(RdataSetTest, mergeCreate) {
             // Create old rdataset
             SegmentObjectHolder<RdataSet, RRClass> holder1(
                 mem_sgmt_,
-                RdataSet::create(mem_sgmt_, encoder_,
-                                 (i & 1) != 0 ? a_rrsets[0] : null_rrset,
-                                 (i & 2) != 0 ? rrsig_rrsets[0] : null_rrset),
                 rrclass);
+            holder1.set(RdataSet::create(mem_sgmt_, encoder_,
+                                 (i & 1) != 0 ? a_rrsets[0] : null_rrset,
+                                 (i & 2) != 0 ? rrsig_rrsets[0] : null_rrset));
             // Create merged rdataset, based on the old one and RRsets
             SegmentObjectHolder<RdataSet, RRClass> holder2(
                 mem_sgmt_,
-                RdataSet::create(mem_sgmt_, encoder_,
+                rrclass);
+            holder2.set(RdataSet::create(mem_sgmt_, encoder_,
                                  (j & 1) != 0 ? a_rrsets[1] : null_rrset,
                                  (j & 2) != 0 ? rrsig_rrsets[1] : null_rrset,
-                                 holder1.get()),
-                rrclass);
+                                 holder1.get()));
 
             // Set up the expected data for the case.
             vector<string> expected_rdata;
@@ -242,15 +242,15 @@ TEST_F(RdataSetTest, duplicate) {
     // After suppressing duplicates, it should be the same as the default
     // RdataSet.  Check that.
     SegmentObjectHolder<RdataSet, RRClass> holder1(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, dup_rrset, dup_rrsig), rrclass);
+        mem_sgmt_, rrclass);
+    holder1.set(RdataSet::create(mem_sgmt_, encoder_, dup_rrset, dup_rrsig));
     checkRdataSet(*holder1.get(), def_rdata_txt_, def_rrsig_txt_);
 
     // Confirm the same thing for the merge mode.
     SegmentObjectHolder<RdataSet, RRClass> holder2(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, a_rrset_, rrsig_rrset_,
-                         holder1.get()), rrclass);
+        mem_sgmt_, rrclass);
+    holder2.set(RdataSet::create(mem_sgmt_, encoder_, a_rrset_, rrsig_rrset_,
+                                 holder1.get()));
     checkRdataSet(*holder2.get(), def_rdata_txt_, def_rrsig_txt_);
 }
 
@@ -276,23 +276,24 @@ TEST_F(RdataSetTest, getNext) {
 TEST_F(RdataSetTest, find) {
     // Create some RdataSets and make a chain of them.
     SegmentObjectHolder<RdataSet, RRClass> holder1(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, a_rrset_, ConstRRsetPtr()),
-        RRClass::IN());
+        mem_sgmt_, RRClass::IN());
+    holder1.set(RdataSet::create(mem_sgmt_, encoder_, a_rrset_,
+                                 ConstRRsetPtr()));
     ConstRRsetPtr aaaa_rrset =
         textToRRset("www.example.com. 1076895760 IN AAAA 2001:db8::1");
     SegmentObjectHolder<RdataSet, RRClass> holder2(
         mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, aaaa_rrset, ConstRRsetPtr()),
         RRClass::IN());
+    holder2.set(RdataSet::create(mem_sgmt_, encoder_, aaaa_rrset,
+                                 ConstRRsetPtr()));
     ConstRRsetPtr sigonly_rrset =
         textToRRset("www.example.com. 1076895760 IN RRSIG "
                     "TXT 5 2 3600 20120814220826 20120715220826 "
                     "1234 example.com. FAKE");
     SegmentObjectHolder<RdataSet, RRClass> holder3(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, ConstRRsetPtr(), sigonly_rrset),
-        RRClass::IN());
+        mem_sgmt_, RRClass::IN());
+    holder3.set(RdataSet::create(mem_sgmt_, encoder_, ConstRRsetPtr(),
+                                 sigonly_rrset));
 
     RdataSet* rdataset_a = holder1.get();
     RdataSet* rdataset_aaaa = holder2.get();
@@ -377,9 +378,8 @@ TEST_F(RdataSetTest, createManyRRs) {
 TEST_F(RdataSetTest, mergeCreateManyRRs) {
     ConstRRsetPtr rrset = textToRRset("example.com. 3600 IN TXT some-text");
     SegmentObjectHolder<RdataSet, RRClass> holder(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, rrset, ConstRRsetPtr()),
-        RRClass::IN());
+        mem_sgmt_, RRClass::IN());
+    holder.set(RdataSet::create(mem_sgmt_, encoder_, rrset, ConstRRsetPtr()));
 
     checkCreateManyRRs(boost::bind(&RdataSet::create, _1, _2, _3, _4,
                                    holder.get()), rrset->getRdataCount());
@@ -475,9 +475,8 @@ TEST_F(RdataSetTest, mergeCreateManyRRSIGs) {
         "example.com. 3600 IN RRSIG A 5 2 3600 20120814220826 20120715220826 "
         "1234 example.com. FAKEFAKE");
     SegmentObjectHolder<RdataSet, RRClass> holder(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, ConstRRsetPtr(), rrsig),
-        rrclass);
+        mem_sgmt_, rrclass);
+    holder.set(RdataSet::create(mem_sgmt_, encoder_, ConstRRsetPtr(), rrsig));
 
     checkCreateManyRRSIGs(boost::bind(&RdataSet::create, _1, _2, _3, _4,
                                       holder.get()), rrsig->getRdataCount());
@@ -544,11 +543,10 @@ TEST_F(RdataSetTest, badMergeCreate) {
     // The 'old RdataSet' for merge.  Its content doesn't matter much; the test
     // should trigger exception before examining it except for the last checks.
     SegmentObjectHolder<RdataSet, RRClass> holder(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_,
+        mem_sgmt_, RRClass::IN());
+    holder.set(RdataSet::create(mem_sgmt_, encoder_,
                          textToRRset("www.example.com. 0 IN AAAA 2001:db8::1"),
-                         ConstRRsetPtr()),
-        RRClass::IN());
+                         ConstRRsetPtr()));
 
     checkBadCreate(boost::bind(&RdataSet::create, _1, _2, _3, _4,
                                holder.get()));
@@ -586,8 +584,8 @@ TEST_F(RdataSetTest, varyingTTL) {
 
     // RRSIG's TTL is smaller
     SegmentObjectHolder<RdataSet, RRClass> holder1(
-        mem_sgmt_,
-        RdataSet::create(mem_sgmt_, encoder_, aaaa_large, sig_small), rrclass);
+        mem_sgmt_, rrclass);
+    holder1.set(RdataSet::create(mem_sgmt_, encoder_, aaaa_large, sig_small));
     EXPECT_EQ(RRTTL(10), restoreTTL(holder1.get()->getTTLData()));
 
     // Merging another RRset (w/o sig) that has larger TTL

src/lib/datasrc/tests/memory/rrset_collection_unittest.cc

@@ -24,6 +24,8 @@
 
 #include <gtest/gtest.h>
 
+#include <boost/scoped_ptr.hpp>
+
 using namespace isc::dns;
 using namespace isc::dns::rdata;
 using namespace std;
@@ -43,22 +45,24 @@ public:
         rrclass("IN"),
         origin("example.org"),
         zone_file(TEST_DATA_DIR "/rrset-collection.zone"),
-        zone_data_holder(mem_sgmt,
-                         loadZoneData(mem_sgmt, rrclass, origin, zone_file),
-                         rrclass),
-        collection(*zone_data_holder.get(), rrclass)
-    {}
+        zone_data_holder(mem_sgmt, rrclass)
+    {
+        zone_data_holder.set(loadZoneData(mem_sgmt, rrclass, origin,
+                                          zone_file));
+        collection.reset(new RRsetCollection(*zone_data_holder.get(),
+                                             rrclass));
+    }
 
     const RRClass rrclass;
     const Name origin;
     std::string zone_file;
     test::MemorySegmentMock mem_sgmt;
     SegmentObjectHolder<ZoneData, RRClass> zone_data_holder;
-    RRsetCollection collection;
+    boost::scoped_ptr<RRsetCollection> collection;
 };
 
 TEST_F(RRsetCollectionTest, find) {
-    const RRsetCollection& ccln = collection;
+    const RRsetCollection& ccln = *collection;
     ConstRRsetPtr rrset = ccln.find(Name("www.example.org"), rrclass,
                                     RRType::A());
     EXPECT_TRUE(rrset);

src/lib/datasrc/tests/memory/segment_object_holder_unittest.cc

@@ -34,6 +34,7 @@ public:
     static void destroy(MemorySegment& sgmt, TestObject* obj, int arg) {
         sgmt.deallocate(obj, sizeof(*obj));
         EXPECT_EQ(TEST_ARG_VAL, arg);
+        EXPECT_TRUE(obj);
     }
 };
 
@@ -46,7 +47,8 @@ useHolder(MemorySegment& sgmt, TestObject* obj, bool release) {
     // deallocate().
 
     typedef SegmentObjectHolder<TestObject, int> HolderType;
-    HolderType holder(sgmt, obj, TEST_ARG_VAL);
+    HolderType holder(sgmt, TEST_ARG_VAL);
+    holder.set(obj);
     EXPECT_EQ(obj, holder.get());
     if (release) {
         EXPECT_EQ(obj, holder.release());
@@ -69,6 +71,16 @@ TEST(SegmentObjectHolderTest, foo) {
     EXPECT_TRUE(sgmt.allMemoryDeallocated());
 }
 
+// Test nothing bad happens if the holder is not set before it is destroyed
+TEST(SegmentObjectHolderTest, destroyNotSet) {
+    MemorySegmentLocal sgmt;
+    {
+        typedef SegmentObjectHolder<TestObject, int> HolderType;
+        HolderType holder(sgmt, TEST_ARG_VAL);
+    }
+    EXPECT_TRUE(sgmt.allMemoryDeallocated());
+}
+
 // Keep allocating bigger and bigger chunks of data until the allocation
 // fails with growing the segment.
 void
@@ -78,7 +90,8 @@ allocateUntilGrows(MemorySegment& segment, size_t& current_size) {
     // the position moved.
     void *object_memory = segment.allocate(sizeof(TestObject));
     TestObject* object = new(object_memory) TestObject;
-    SegmentObjectHolder<TestObject, int> holder(segment, object, TEST_ARG_VAL);
+    SegmentObjectHolder<TestObject, int> holder(segment, TEST_ARG_VAL);
+    holder.set(object);
     while (true) {
         void* data = segment.allocate(current_size);
         segment.deallocate(data, current_size);

src/lib/datasrc/tests/memory/zone_data_loader_unittest.cc

@@ -99,8 +99,8 @@ TEST(ZoneDataLoaterTest, relocate) {
                                       TEST_DATA_DIR
                                       "/example.org-nsec3-signed.zone");
         // Store it, so it is cleaned up later
-        zones.push_back(HolderPtr(new Holder(segment, data,
-                                             RRClass::IN())));
+        zones.push_back(HolderPtr(new Holder(segment, RRClass::IN())));
+        zones.back()->set(data);
 
     }
     // Deallocate all the zones now.

src/lib/datasrc/tests/memory/zone_table_unittest.cc

@@ -79,7 +79,8 @@ TEST_F(ZoneTableTest, addZone) {
     EXPECT_EQ(0, zone_table->getZoneCount()); // count is still 0
 
     SegmentObjectHolder<ZoneData, RRClass> holder1(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_);
+        mem_sgmt_, zclass_);
+    holder1.set(ZoneData::create(mem_sgmt_, zname1));
     const ZoneData* data1(holder1.get());
     // Normal successful case.
     const ZoneTable::AddResult result1(zone_table->addZone(mem_sgmt_, zclass_,
@@ -93,7 +94,8 @@ TEST_F(ZoneTableTest, addZone) {
 
     // Duplicate add doesn't replace the existing data.
     SegmentObjectHolder<ZoneData, RRClass> holder2(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_);
+        mem_sgmt_, zclass_);
+    holder2.set(ZoneData::create(mem_sgmt_, zname1));
     const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zclass_,
                                                            zname1,
                                                            holder2.release()));
@@ -107,8 +109,8 @@ TEST_F(ZoneTableTest, addZone) {
     EXPECT_EQ(1, zone_table->getZoneCount()); // count doesn't change.
 
     SegmentObjectHolder<ZoneData, RRClass> holder3(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, Name("EXAMPLE.COM")),
-                                    zclass_);
+        mem_sgmt_, zclass_);
+    holder3.set(ZoneData::create(mem_sgmt_, Name("EXAMPLE.COM")));
     // names are compared in a case insensitive manner.
     const ZoneTable::AddResult result3(zone_table->addZone(mem_sgmt_, zclass_,
                                                            Name("EXAMPLE.COM"),
@@ -117,13 +119,15 @@ TEST_F(ZoneTableTest, addZone) {
     ZoneData::destroy(mem_sgmt_, result3.zone_data, zclass_);
     // Add some more different ones.  Should just succeed.
     SegmentObjectHolder<ZoneData, RRClass> holder4(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_);
+        mem_sgmt_, zclass_);
+    holder4.set(ZoneData::create(mem_sgmt_, zname2));
     EXPECT_EQ(result::SUCCESS,
               zone_table->addZone(mem_sgmt_, zclass_, zname2,
                                   holder4.release()).code);
     EXPECT_EQ(2, zone_table->getZoneCount());
     SegmentObjectHolder<ZoneData, RRClass> holder5(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_);
+        mem_sgmt_, zclass_);
+    holder5.set(ZoneData::create(mem_sgmt_, zname3));
     EXPECT_EQ(result::SUCCESS,
               zone_table->addZone(mem_sgmt_, zclass_, zname3,
                                   holder5.release()).code);
@@ -133,7 +137,8 @@ TEST_F(ZoneTableTest, addZone) {
     // tree.  It still shouldn't cause memory leak (which would be detected
     // in TearDown()).
     SegmentObjectHolder<ZoneData, RRClass> holder6(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, Name("example.org")), zclass_);
+        mem_sgmt_, zclass_);
+    holder6.set(ZoneData::create(mem_sgmt_, Name("example.org")));
     mem_sgmt_.setThrowCount(1);
     EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, Name("example.org"),
                                      holder6.release()),
@@ -142,17 +147,20 @@ TEST_F(ZoneTableTest, addZone) {
 
 TEST_F(ZoneTableTest, findZone) {
     SegmentObjectHolder<ZoneData, RRClass> holder1(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_);
+        mem_sgmt_, zclass_);
+    holder1.set(ZoneData::create(mem_sgmt_, zname1));
     ZoneData* zone_data = holder1.get();
     EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname1,
                                                    holder1.release()).code);
     SegmentObjectHolder<ZoneData, RRClass> holder2(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_);
+        mem_sgmt_, zclass_);
+    holder2.set(ZoneData::create(mem_sgmt_, zname2));
     EXPECT_EQ(result::SUCCESS,
               zone_table->addZone(mem_sgmt_, zclass_, zname2,
                                   holder2.release()).code);
     SegmentObjectHolder<ZoneData, RRClass> holder3(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_);
+        mem_sgmt_, zclass_);
+    holder3.set(ZoneData::create(mem_sgmt_, zname3));
     EXPECT_EQ(result::SUCCESS,
               zone_table->addZone(mem_sgmt_, zclass_, zname3,
                                   holder3.release()).code);
@@ -177,7 +185,8 @@ TEST_F(ZoneTableTest, findZone) {
     // make sure the partial match is indeed the longest match by adding
     // a zone with a shorter origin and query again.
     SegmentObjectHolder<ZoneData, RRClass> holder4(
-        mem_sgmt_, ZoneData::create(mem_sgmt_, Name("com")), zclass_);
+        mem_sgmt_, zclass_);
+    holder4.set(ZoneData::create(mem_sgmt_, Name("com")));
     EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_,
                                                    Name("com"),
                                                    holder4.release()).code);