[2100] improved exception safety.

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

@@ -25,11 +25,34 @@
 
 #include <gtest/gtest.h>
 
+#include <new>                  // for bad_alloc
+
 using namespace isc::dns;
 using namespace isc::datasrc;
 using namespace isc::datasrc::memory;
 
 namespace {
+// Memory segment specified for tests.  It normally behaves like a "local"
+// memory segment.  If "throw count" is set to non 0 via setThrowCount(),
+// it continues the normal behavior up to the specified number of calls to
+// allocate(), and throws an exception at the next call.
+class TestMemorySegment : public isc::util::MemorySegmentLocal {
+public:
+    TestMemorySegment() : throw_count_(0) {}
+    virtual void* allocate(size_t size) {
+        if (throw_count_ > 0) {
+            if (--throw_count_ == 0) {
+                throw std::bad_alloc();
+            }
+        }
+        return (isc::util::MemorySegmentLocal::allocate(size));
+    }
+    void setThrowCount(size_t count) { throw_count_ = count; }
+
+private:
+    size_t throw_count_;
+};
+
 class ZoneTableTest : public ::testing::Test {
 protected:
     ZoneTableTest() : zname1(Name("example.com")),
@@ -45,13 +68,23 @@ protected:
     void TearDown() {
         ZoneTable::destroy(mem_sgmt_, zone_table);
         zone_table = NULL;
-        EXPECT_TRUE(mem_sgmt_.allMemoryDeallocated());
+        EXPECT_TRUE(mem_sgmt_.allMemoryDeallocated()); // catch any leak here.
     }
     const Name zname1, zname2, zname3;
-    isc::util::MemorySegmentLocal mem_sgmt_;
+    TestMemorySegment mem_sgmt_;
     ZoneTable* zone_table;
 };
 
+TEST_F(ZoneTableTest, create) {
+    // Test about creating a zone table.  Normal case covers through other
+    // tests.  We only check exception safety by letting the test memory
+    // segment throw.
+    mem_sgmt_.setThrowCount(2);
+    ZoneTable* table;
+    EXPECT_THROW(table = ZoneTable::create(mem_sgmt_), std::bad_alloc);
+    // This shouldn't cause memory leak (that would be caught in TearDown()).
+}
+
 TEST_F(ZoneTableTest, addZone) {
     EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1));
     EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zname1));
@@ -61,6 +94,13 @@ TEST_F(ZoneTableTest, addZone) {
     // Add some more different ones.  Should just succeed.
     EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2));
     EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3));
+
+    // Have the memory segment throw an exception in extending the internal
+    // tree.  It still shouldn't cause memory leak (which would be detected
+    // in TearDown()).
+    mem_sgmt_.setThrowCount(2);
+    EXPECT_THROW(zone_table->addZone(mem_sgmt_, Name("example.org")),
+                 std::bad_alloc);
 }
 
 TEST_F(ZoneTableTest, DISABLED_removeZone) {

src/lib/datasrc/memory/zone_data.h

@@ -22,10 +22,22 @@ namespace isc {
 namespace datasrc {
 namespace memory {
 class ZoneData {
-public:
+private:
     ZoneData(const dns::Name& zone_name) :
         zone_name_(zone_name)
     {}
+public:
+    static ZoneData* create(util::MemorySegment& mem_sgmt,
+                            const dns::Name& zone_name)
+    {
+        void* p = mem_sgmt.allocate(sizeof(ZoneData));
+        ZoneData* zone_data = new(p) ZoneData(zone_name);
+        return (zone_data);
+    }
+    static void destroy(util::MemorySegment& mem_sgmt, ZoneData* zone_data) {
+        zone_data->~ZoneData();
+        mem_sgmt.deallocate(zone_data, sizeof(ZoneData));
+    }
 
 private:
     const dns::Name zone_name_;

src/lib/datasrc/memory/zone_table.cc

@@ -28,35 +28,38 @@ using namespace isc::dns;
 namespace isc {
 namespace datasrc {
 namespace memory {
-
-ZoneTable::ZoneTable(util::MemorySegment& mem_sgmt) :
-    zones_(ZoneTableTree::create(mem_sgmt))
-{}
-
 namespace {
-// A simple holder to make resource allocation for ZoneTable exception safe.
-// It works like std::auto_ptr but much more simplified.
-struct ZoneTableHolder {
-    ZoneTableHolder(util::MemorySegment& mem_sgmt, void* ptr) :
-        mem_sgmt_(mem_sgmt), ptr_(ptr)
+// A simple holder to create and use some objects in this implementation
+// in an exception safe manner.   It works like std::auto_ptr but much
+// more simplified.
+template <typename T>
+class Holder {
+public:
+    Holder(util::MemorySegment& mem_sgmt, T* obj) :
+        mem_sgmt_(mem_sgmt), obj_(obj)
     {}
-    ~ZoneTableHolder() {
-        if (ptr_ != NULL) {
-            mem_sgmt_.deallocate(ptr_, sizeof(ZoneTable));
+    ~Holder() {
+        if (obj_ != NULL) {
+            T::destroy(mem_sgmt_, obj_);
         }
     }
-    void* get() { return (ptr_); }
-    void release() { ptr_ = NULL; }
+    T* get() { return (obj_); }
+    T* release() {
+        T* ret = obj_;
+        obj_ = NULL;
+        return (ret);
+    }
 private:
     util::MemorySegment& mem_sgmt_;
-    void* ptr_;
+    T* obj_;
 };
 }
 
 ZoneTable*
 ZoneTable::create(util::MemorySegment& mem_sgmt) {
-    ZoneTableHolder holder(mem_sgmt, mem_sgmt.allocate(sizeof(ZoneTable)));
-    ZoneTable* zone_table = new(holder.get()) ZoneTable(mem_sgmt);
+    Holder<ZoneTableTree> holder(mem_sgmt, ZoneTableTree::create(mem_sgmt));
+    void* p = mem_sgmt.allocate(sizeof(ZoneTable));
+    ZoneTable* zone_table = new(p) ZoneTable(holder.get());
     holder.release();
     return (zone_table);
 }
@@ -69,13 +72,13 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable) {
 
 result::Result
 ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name) {
-    // Create a new ZoneData instance first.  If it already exists the new
-    // one will soon be destroyed, but we want to make sure if this allocation
-    // fails the tree won't be changed to provide as strong guarantee as
-    // possible.  In practice, we generally expect the caller tries to add
-    // a zone only when it's a new one, so this should be a minor concern.
-    void* p = mem_sgmt.allocate(sizeof(ZoneData)); // XXX safety
-    ZoneData* zone_data = new(p) ZoneData(zone_name);
+    // Create a new ZoneData instance first.  If the specified name already
+    // exists in the table, the new data will soon be destroyed, but we want
+    // to make sure if this allocation fails the tree won't be changed to
+    // provide as strong guarantee as possible.  In practice, we generally
+    // expect the caller tries to add a zone only when it's a new one, so
+    // this should be a minor concern.
+    Holder<ZoneData> holder(mem_sgmt, ZoneData::create(mem_sgmt, zone_name));
 
     // Get the node where we put the zone
     ZoneTableNode* node(NULL);
@@ -93,10 +96,9 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name) {
 
     // Is it empty? We either just created it or it might be nonterminal
     if (node->isEmpty()) {
-        node->setData(mem_sgmt, zone_data);
+        node->setData(mem_sgmt, holder.release());
         return (result::SUCCESS);
     } else { // There's something there already
-        mem_sgmt.deallocate(p, sizeof(ZoneData));
         return (result::EXIST);
     }
 }

src/lib/datasrc/memory/zone_table.h

@@ -45,6 +45,20 @@ class ZoneData;
 /// For more descriptions about its struct and interfaces, please refer to the
 /// corresponding struct and interfaces of \c MemoryDataSrc.
 class ZoneTable {
+private:
+    struct ZoneDataDeleter {
+        ZoneDataDeleter() {}
+        void operator()(util::MemorySegment& mem_sgmt,
+                        ZoneData* zone_data) const
+        {
+            mem_sgmt.deallocate(zone_data, sizeof(ZoneData));
+        }
+    };
+
+    // Type aliases to make it shorter
+    typedef DomainTree<ZoneData, ZoneDataDeleter> ZoneTableTree;
+    typedef DomainTreeNode<ZoneData, ZoneDataDeleter> ZoneTableNode;
+
 public:
     struct FindResult {
         FindResult(result::Result param_code, const ZoneFinderPtr param_zone) :
@@ -72,7 +86,8 @@ private:
     /// This constructor internally involves resource allocation, and if
     /// it fails, a corresponding standard exception will be thrown.
     /// It never throws an exception otherwise.
-    ZoneTable(util::MemorySegment& mem_sgmt);
+    ZoneTable(ZoneTableTree* zones) : zones_(zones)
+    {}
     //@}
 
 public:
@@ -143,19 +158,6 @@ public:
     FindResult findZone(const isc::dns::Name& name) const;
 
 private:
-    struct ZoneDataDeleter {
-        ZoneDataDeleter() {}
-        void operator()(util::MemorySegment& mem_sgmt,
-                        ZoneData* zone_data) const
-        {
-            mem_sgmt.deallocate(zone_data, sizeof(ZoneData));
-        }
-    };
-
-    // Type aliases to make it shorter
-    typedef DomainTree<ZoneData, ZoneDataDeleter> ZoneTableTree;
-    typedef DomainTreeNode<ZoneData, ZoneDataDeleter> ZoneTableNode;
-
     ZoneTableTree* zones_;
 };
 }