Browse Source

[2836-2] make sure remapping boost segment even if grow() fails.

otherwise, subsequent cleanup to be exception safe will cause a crash.
and, if the remap fails simply abort(); there's no hope to recover from
this situation reasonably.  and clarify in shrinkToFit() why it behaves
differently.
JINMEI Tatuya 12 years ago
parent
commit
3a6ae66c2a

+ 19 - 9
src/lib/util/memory_segment_mapped.cc

@@ -140,16 +140,21 @@ struct MemorySegmentMapped::Impl {
         const size_t new_size = prev_size * 2;
         assert(new_size > prev_size);
 
-        if (!BaseSegment::grow(filename_.c_str(), new_size - prev_size)) {
-            throw std::bad_alloc();
-        }
-
+        const bool grown = BaseSegment::grow(filename_.c_str(),
+                                             new_size - prev_size);
+
+        // Remap the file, whether or not grow() succeeded.  this should
+        // normally succeed(*), but it's not 100% guaranteed.  We abort
+        // if it fails (see the method description in the header file).
+        // (*) Although it's not formally documented, the implementation
+        // of grow() seems to provide strong guarantee, i.e, if it fails
+        // the underlying file can be used with the previous size.
         try {
-            // Remap the grown file; this should succeed, but it's not 100%
-            // guaranteed.  If it fails we treat it as if we fail to create
-            // the new segment.
             base_sgmt_.reset(new BaseSegment(open_only, filename_.c_str()));
-        } catch (const boost::interprocess::interprocess_exception& ex) {
+        } catch (...) {
+            abort();
+        }
+        if (!grown) {
             throw std::bad_alloc();
         }
     }
@@ -350,7 +355,12 @@ MemorySegmentMapped::shrinkToFit() {
     try {
         // Remap the shrunk file; this should succeed, but it's not 100%
         // guaranteed.  If it fails we treat it as if we fail to create
-        // the new segment.
+        // the new segment.  Note that this is different from the case where
+        // reset() after grow() fails.  While the same argument can apply
+        // in theory, it should be less likely that other methods will be
+        // called after shrinkToFit() (and the destructor can still be called
+        // safely), so we give the application an opportunity to handle the
+        // case as gracefully as possible.
         impl_->base_sgmt_.reset(
             new BaseSegment(open_only, impl_->filename_.c_str()));
     } catch (const boost::interprocess::interprocess_exception& ex) {

+ 8 - 1
src/lib/util/memory_segment_mapped.h

@@ -147,7 +147,14 @@ public:
 
     /// \brief Allocate/acquire a segment of memory.
     ///
-    /// This version can throw \c MemorySegmentGrown.
+    /// This version can throw \c MemorySegmentGrown.  Furthermore, there is
+    /// a very small chance that the object loses its integrity and can't be
+    /// usable in the case where \c MemorySegmentGrown would be thrown.
+    /// In this case, throwing a different exception wouldn't help, because
+    /// an application trying to provide exception safety might then call
+    /// deallocate() or named address APIs on this object, which would simply
+    /// cause a crash.  So, while suboptimal, this method just aborts the
+    /// program in this case, indicating there's no hope to shutdown cleanly.
     ///
     /// This method cannot be called if the segment object is created in the
     /// read-only mode; in that case MemorySegmentError will be thrown.

+ 3 - 2
src/lib/util/tests/memory_segment_mapped_unittest.cc

@@ -238,11 +238,12 @@ TEST_F(MemorySegmentMappedTest, allocate) {
 
 TEST_F(MemorySegmentMappedTest, badAllocate) {
     // Make the mapped file non-writable; managed_mapped_file::grow() will
-    // fail, resulting in std::bad_alloc
+    // fail, resulting in abort.
     const int ret = chmod(mapped_file, 0444);
     ASSERT_EQ(0, ret);
 
-    EXPECT_THROW(segment_->allocate(DEFAULT_INITIAL_SIZE * 2), std::bad_alloc);
+    EXPECT_DEATH_IF_SUPPORTED(
+        {segment_->allocate(DEFAULT_INITIAL_SIZE * 2);}, "");
 }
 
 // XXX: this test can cause too strong side effect (creating a very large