[2850] Implement the strong exception guarantee

src/lib/datasrc/memory/zone_table_segment.h

@@ -73,19 +73,6 @@ public:
     {}
 };
 
-/// \brief Exception thrown when a \c reset() on a ZoneTableSegment
-/// fails because it was determined that the backing memory store is
-/// corrupted. This is typically an unexpected condition that may arise
-/// in rare cases. When this exception is thrown, there is a strong
-/// guarantee that the previously existing backing memory store was
-/// cleared.
-class BrokenSegment : public ResetFailedAndSegmentCleared {
-public:
-    BrokenSegment(const char* file, size_t line, const char* what) :
-        ResetFailedAndSegmentCleared(file, line, what)
-    {}
-};
-
 /// \brief Memory-management independent entry point that contains a
 /// pointer to a zone table in memory.
 ///
@@ -214,6 +201,26 @@ public:
     /// \brief Unload the current memory store (if loaded) and load the
     /// specified one.
     ///
+    /// In case opening/loading the new memory store fails for some
+    /// reason, one of the following documented (further below)
+    /// exceptions may be thrown. In case failures occur,
+    /// implementations of this method must strictly provide the
+    /// associated behavior as follows, and in the exception
+    /// documentation below.  Code that uses \c ZoneTableSegment would
+    /// depend on such assurances.
+    ///
+    /// In case an existing memory store is in use, and an attempt to
+    /// load a different memory store fails, the existing memory store
+    /// must still be available and the \c ResetFailed exception must be
+    /// thrown. In this case, the segment is still usable.
+    ///
+    /// In case an existing memory store is in use, and an attempt is
+    /// made to reload the same memory store which results in a failure,
+    /// the existing memory store must no longer be available and the
+    /// \c ResetFailedAndSegmentCleared exception must be thrown. In
+    /// this case, the segment is no longer usable without a further
+    /// successful call to \c reset().
+    ///
     /// See the \c MemorySegmentOpenMode documentation above for the
     /// various modes in which a ZoneTableSegment can be created.
     ///
@@ -223,10 +230,17 @@ public:
     /// argument.
     ///
     /// \throws isc::InvalidParameter if the configuration in \c params
-    /// has incorrect syntax.
-    /// \throws isc::Unexpected for a variety of cases where an
-    /// unexpected condition occurs. These should not occur normally in
-    /// correctly written code.
+    /// has incorrect syntax, but the segment is still usable due to the
+    /// old memory store still being in use.
+    ///
+    /// \throw ResetFailed if there was a problem in loading the new
+    /// memory store, but the segment is still usable due to the old
+    /// memory store still being in use.
+    ///
+    /// \throw ResetFailedAndSegmentCleared if there was a problem in
+    /// loading the new memory store, but the old memory store was also
+    /// unloaded and is no longer in use. The segment is not usable
+    /// without a further successful \c reset().
     ///
     /// \param mode The open mode (see the MemorySegmentOpenMode
     /// documentation).

src/lib/datasrc/memory/zone_table_segment_mapped.cc

@@ -43,16 +43,17 @@ ZoneTableSegmentMapped::ZoneTableSegmentMapped(const RRClass& rrclass) :
 
 bool
 ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
-                                        bool createMode)
+                                        bool create,
+                                        std::string& error_msg)
 {
     MemorySegment::NamedAddressResult result =
         segment.getNamedAddress(ZONE_TABLE_CHECKSUM_NAME);
     if (result.first) {
-        if (createMode) {
+        if (create) {
             // There must be no previously saved checksum.
-            isc_throw(isc::Unexpected,
-                      "There is already a saved checksum in a mapped segment "
-                      "opened in create mode.");
+            error_msg = "There is already a saved checksum in the segment "
+                 "opened in create mode";
+            return (false);
         } else {
             // The segment was already shrunk when it was last
             // closed. Check that its checksum is consistent.
@@ -64,11 +65,10 @@ ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
             *checksum = 0;
             const uint32_t new_checksum = segment.getCheckSum();
             if (saved_checksum != new_checksum) {
-                isc_throw(isc::Unexpected,
-                          "Saved checksum doesn't match mapped segment data");
+                error_msg = "Saved checksum doesn't match segment data";
+                return (false);
             }
         }
-        return (true);
     } else {
         // Allocate space for a checksum (which is saved during close).
 
@@ -87,27 +87,34 @@ ZoneTableSegmentMapped::processChecksum(MemorySegmentMapped& segment,
         *static_cast<uint32_t*>(checksum) = 0;
         const bool grew = segment.setNamedAddress(ZONE_TABLE_CHECKSUM_NAME,
                                                   checksum);
-        // If the segment grew here, we have a problem as the checksum
-        // address may no longer be valid. In this case, we cannot
-        // recover. This case is extremely unlikely as we reserved
-        // memory for the ZONE_TABLE_CHECKSUM_NAME above. It indicates a
-        // very restrictive MemorySegment which we should not use.
-        return (!grew);
+        if (grew) {
+            // If the segment grew here, we have a problem as the
+            // checksum address may no longer be valid. In this case, we
+            // cannot recover. This case is extremely unlikely as we
+            // reserved memory for the ZONE_TABLE_CHECKSUM_NAME
+            // above. It indicates a very restrictive MemorySegment
+            // which we should not use.
+            error_msg = "Segment grew unexpectedly in setNamedAddress()";
+            return (false);
+        }
     }
+
+    return (true);
 }
 
 bool
 ZoneTableSegmentMapped::processHeader(MemorySegmentMapped& segment,
-                                      bool createMode)
+                                      bool create,
+                                      std::string& error_msg)
 {
     MemorySegment::NamedAddressResult result =
         segment.getNamedAddress(ZONE_TABLE_HEADER_NAME);
     if (result.first) {
-        if (createMode) {
+        if (create) {
             // There must be no previously saved checksum.
-            isc_throw(isc::Unexpected,
-                      "There is already a saved ZoneTableHeader in a "
-                      "mapped segment opened in create mode.");
+            error_msg = "There is already a saved ZoneTableHeader in the "
+                 "segment opened in create mode";
+            return (false);
         } else {
             assert(result.second);
             header_ = static_cast<ZoneTableHeader*>(result.second);
@@ -127,42 +134,57 @@ ZoneTableSegmentMapped::processHeader(MemorySegmentMapped& segment,
         const bool grew = segment.setNamedAddress(ZONE_TABLE_HEADER_NAME,
                                                   new_header);
         if (grew) {
-             // If the segment grew here, we have a problem as the table
-             // header address may no longer be valid. In this case, we
-             // cannot recover. This case is extremely unlikely as we
-             // reserved memory for the ZONE_TABLE_HEADER_NAME above. It
-             // indicates a very restrictive MemorySegment which we
-             // should not use.
-             return (false);
+            // If the segment grew here, we have a problem as the table
+            // header address may no longer be valid. In this case, we
+            // cannot recover. This case is extremely unlikely as we
+            // reserved memory for the ZONE_TABLE_HEADER_NAME above. It
+            // indicates a very restrictive MemorySegment which we
+            // should not use.
+            error_msg = "Segment grew unexpectedly in setNamedAddress()";
+            return (false);
         }
         header_ = new_header;
     }
-    return (true);
-}
-
-void
-ZoneTableSegmentMapped::openCreate(const std::string& filename) {
-    // In case there is a checksum mismatch, we throw. We want the
-    // segment to be automatically destroyed then.
-    std::auto_ptr<MemorySegmentMapped> segment
-        (new MemorySegmentMapped(filename, MemorySegmentMapped::CREATE_ONLY));
 
-    processChecksum(*segment, true);
-    processHeader(*segment, true);
-
-    mem_sgmt_.reset(segment.release());
+    return (true);
 }
 
 void
-ZoneTableSegmentMapped::openReadWrite(const std::string& filename) {
-    // In case there is a checksum mismatch, we throw. We want the
-    // segment to be automatically destroyed then.
+ZoneTableSegmentMapped::openReadWrite(const std::string& filename,
+                                      bool create)
+{
+    const MemorySegmentMapped::OpenMode mode = create ?
+         MemorySegmentMapped::CREATE_ONLY :
+         MemorySegmentMapped::OPEN_OR_CREATE;
+    // In case there is a problem, we throw. We want the segment to be
+    // automatically destroyed then.
     std::auto_ptr<MemorySegmentMapped> segment
-        (new MemorySegmentMapped(filename,
-                                 MemorySegmentMapped::OPEN_OR_CREATE));
+        (new MemorySegmentMapped(filename, mode));
+
+    std::string error_msg;
+    if (!processChecksum(*segment, create, error_msg)) {
+         if (mem_sgmt_) {
+              isc_throw(ResetFailed,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         } else {
+              isc_throw(ResetFailedAndSegmentCleared,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         }
+    }
 
-    processChecksum(*segment, false);
-    processHeader(*segment, false);
+    if (!processHeader(*segment, create, error_msg)) {
+         if (mem_sgmt_) {
+              isc_throw(ResetFailed,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         } else {
+              isc_throw(ResetFailedAndSegmentCleared,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         }
+    }
 
     mem_sgmt_.reset(segment.release());
 }
@@ -177,9 +199,18 @@ ZoneTableSegmentMapped::openReadOnly(const std::string& filename) {
     MemorySegment::NamedAddressResult result =
         segment->getNamedAddress(ZONE_TABLE_CHECKSUM_NAME);
     if (!result.first) {
-        isc_throw(isc::Unexpected,
-                  "There is no previously saved checksum in a "
-                  "mapped segment opened in read-only mode.");
+         const std::string error_msg =
+             "There is no previously saved checksum in a "
+             "mapped segment opened in read-only mode";
+         if (mem_sgmt_) {
+              isc_throw(ResetFailed,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         } else {
+              isc_throw(ResetFailedAndSegmentCleared,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         }
     }
 
     // We can't verify the checksum here as we can't set the checksum to
@@ -192,9 +223,18 @@ ZoneTableSegmentMapped::openReadOnly(const std::string& filename) {
         assert(result.second);
         header_ = static_cast<ZoneTableHeader*>(result.second);
     } else {
-        isc_throw(isc::Unexpected,
-                  "There is no previously saved ZoneTableHeader in a "
-                  "mapped segment opened in read-only mode.");
+         const std::string error_msg =
+             "There is no previously saved ZoneTableHeader in a "
+             "mapped segment opened in read-only mode.";
+         if (mem_sgmt_) {
+              isc_throw(ResetFailed,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         } else {
+              isc_throw(ResetFailedAndSegmentCleared,
+                        "Error in resetting zone table segment to use "
+                        << filename << ": " << error_msg);
+         }
     }
 
     mem_sgmt_.reset(segment.release());
@@ -204,8 +244,6 @@ void
 ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
                               isc::data::ConstElementPtr params)
 {
-    clear();
-
     if (!params || params->getType() != Element::map) {
         isc_throw(isc::InvalidParameter,
                   "Configuration does not contain a map");
@@ -224,13 +262,20 @@ ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
 
     const std::string filename = mapped_file->stringValue();
 
+    if (mem_sgmt_ && (filename == current_filename_)) {
+        // This reset() is an attempt to re-open the currently open
+        // mapped file. We cannot do this in many mode combinations
+        // unless we close the existing mapped file. So just close it.
+        clear();
+    }
+
     switch (mode) {
     case CREATE:
-        openCreate(filename);
+        openReadWrite(filename, true);
         break;
 
     case READ_WRITE:
-        openReadWrite(filename);
+        openReadWrite(filename, false);
         break;
 
     case READ_ONLY:
@@ -238,6 +283,7 @@ ZoneTableSegmentMapped::reset(MemorySegmentOpenMode mode,
     }
 
     current_mode_ = mode;
+    current_filename_ = filename;
 }
 
 void

src/lib/datasrc/memory/zone_table_segment_mapped.h

@@ -74,6 +74,10 @@ public:
     /// \brief Unmap the current file (if mapped) and map the specified
     /// one.
     ///
+    /// In case of exceptions, the current existing mapped file may be
+    /// left open, or may be cleared. Please see the \c ZoneTableSegment
+    /// API documentation for the behavior.
+    ///
     /// See the \c MemorySegmentOpenMode documentation (in
     /// \c ZoneTableSegment class) for the various modes in which a
     /// \c ZoneTableSegmentMapped can be created.
@@ -101,16 +105,18 @@ public:
     virtual void clear();
 
 private:
-    bool processChecksum(isc::util::MemorySegmentMapped& segment, bool create);
-    bool processHeader(isc::util::MemorySegmentMapped& segment, bool create);
+    bool processChecksum(isc::util::MemorySegmentMapped& segment, bool create,
+                         std::string& error_msg);
+    bool processHeader(isc::util::MemorySegmentMapped& segment, bool create,
+                       std::string& error_msg);
 
-    void openCreate(const std::string& filename);
-    void openReadWrite(const std::string& filename);
+    void openReadWrite(const std::string& filename, bool create);
     void openReadOnly(const std::string& filename);
 
 private:
     isc::dns::RRClass rrclass_;
     MemorySegmentOpenMode current_mode_;
+    std::string current_filename_;
     // Internally holds a MemorySegmentMapped. This is NULL on
     // construction, and is set by the \c reset() method.
     boost::scoped_ptr<isc::util::MemorySegmentMapped> mem_sgmt_;

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

@@ -151,21 +151,20 @@ TEST_F(ZoneTableSegmentMappedTest, reset) {
                            config_params_);
     EXPECT_TRUE(ztable_segment_->isWritable());
 
-    // When we reset() and it fails, then the segment should be
-    // unusable.
+    // When we reset() with an invalid paramter and it fails, then the
+    // segment should still be usable.
     EXPECT_THROW({
         ztable_segment_->reset(ZoneTableSegment::CREATE,
                                Element::fromJSON("{}"));
     }, isc::InvalidParameter);
-    // The following should throw now.
-    EXPECT_THROW(ztable_segment_->getHeader(), isc::InvalidOperation);
-    EXPECT_THROW(ztable_segment_->getMemorySegment(), isc::InvalidOperation);
-
-    // isWritable() must return false, because the last reset() failed.
-    EXPECT_FALSE(ztable_segment_->isWritable());
+    EXPECT_TRUE(ztable_segment_->isWritable());
+    // The following should not throw.
+    EXPECT_NO_THROW(ztable_segment_->getHeader());
+    EXPECT_NO_THROW(ztable_segment_->getMemorySegment());
 
     // READ_WRITE with an existing map file ought to work too. This
-    // would use existing named addresses.
+    // would use existing named addresses. This actually re-opens the
+    // currently open map.
     ztable_segment_->reset(ZoneTableSegment::READ_WRITE,
                            config_params_);
     EXPECT_TRUE(ztable_segment_->isWritable());