Ensure cluster cache is properly clean in case of exception in constr…

…uctor.
openzim · Feb 27, 2025 · e99f9c0 · e99f9c0
1 parent 64d73e6
commit e99f9c0
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 16 deletions.
diff --git a/src/fileimpl.cpp b/src/fileimpl.cpp
@@ -248,30 +248,41 @@ class Grouping
       const_cast<entry_index_t&>(m_endUserEntry) = getCountArticles();
     }
 
-    auto result = tmpDirentLookup.find('X', "listing/titleOrdered/v1");
-    if (result.first) {
-      mp_titleDirentAccessor = getTitleAccessorV1(result.second);
-    }
+    // Following code will may create cluster and we want to remove them from cache
+    // if something goes wrong.
+    try {
+      auto result = tmpDirentLookup.find('X', "listing/titleOrdered/v1");
+      if (result.first) {
+        mp_titleDirentAccessor = getTitleAccessorV1(result.second);
+      }
 
-    if (!mp_titleDirentAccessor) {
-      if (!header.hasTitleListingV0()) {
-        throw ZimFileFormatError("Zim file doesn't contain a title ordered index");
+      if (!mp_titleDirentAccessor) {
+        if (!header.hasTitleListingV0()) {
+          throw ZimFileFormatError("Zim file doesn't contain a title ordered index");
+        }
+        offset_t titleOffset(header.getTitleIdxPos());
+        zsize_t  titleSize(sizeof(entry_index_type)*header.getArticleCount());
+        mp_titleDirentAccessor = getTitleAccessor(titleOffset, titleSize, "Title index table");
+        const_cast<bool&>(m_hasFrontArticlesIndex) = false;
       }
-      offset_t titleOffset(header.getTitleIdxPos());
-      zsize_t  titleSize(sizeof(entry_index_type)*header.getArticleCount());
-      mp_titleDirentAccessor = getTitleAccessor(titleOffset, titleSize, "Title index table");
-      const_cast<bool&>(m_hasFrontArticlesIndex) = false;
-    }
-    m_byTitleDirentLookup.reset(new ByTitleDirentLookup(mp_titleDirentAccessor.get()));
+      m_byTitleDirentLookup.reset(new ByTitleDirentLookup(mp_titleDirentAccessor.get()));
 
-    readMimeTypes();
+      readMimeTypes();
+    } catch (...) {
+      dropCachedClusters();
+      throw;
+    }
   }
 
   FileImpl::~FileImpl() {
-    // We have to clean the global cache for our clusters.
+    dropCachedClusters();
+  }
+
+  void FileImpl::dropCachedClusters() const {
     getClusterCache().dropAll([=](const std::tuple<FileImpl*, cluster_index_type>& key) {return std::get<0>(key) == this;});
   }
 
+
   std::unique_ptr<IndirectDirentAccessor> FileImpl::getTitleAccessorV1(const entry_index_t idx)
   {
     auto dirent = mp_pathDirentAccessor->getDirent(idx);

diff --git a/src/fileimpl.h b/src/fileimpl.h
@@ -166,6 +166,8 @@ namespace zim
       explicit FileImpl(std::shared_ptr<FileCompound> zimFile);
       FileImpl(std::shared_ptr<FileCompound> zimFile, offset_t offset, zsize_t size);
 
+      void dropCachedClusters() const;
+
       std::unique_ptr<IndirectDirentAccessor> getTitleAccessorV1(const entry_index_t idx);
       std::unique_ptr<IndirectDirentAccessor> getTitleAccessor(const offset_t offset, const zsize_t size, const std::string& name);
 

diff --git a/test/archive.cpp b/test/archive.cpp
@@ -50,6 +50,9 @@ class ZimArchive: public testing::Test {
     zim::setClusterCacheMaxSize(CLUSTER_CACHE_SIZE);
     ASSERT_EQ(zim::getClusterCacheCurrentSize(), 0);
   }
+  void TearDown() override {
+    ASSERT_EQ(zim::getClusterCacheCurrentSize(), 0);
+  }
 };
 
 using TestContextImpl = std::vector<std::pair<std::string, std::string> >;
@@ -691,7 +694,8 @@ class CapturedStderr
 #define EXPECT_BROKEN_ZIMFILE(ZIMPATH, EXPECTED_STDERROR_TEXT) \
   CapturedStderr stderror;                                     \
   EXPECT_FALSE(zim::validate(ZIMPATH, checksToRun));           \
-  EXPECT_EQ(EXPECTED_STDERROR_TEXT, std::string(stderror)) << ZIMPATH;
+  EXPECT_EQ(EXPECTED_STDERROR_TEXT, std::string(stderror)) << ZIMPATH; \
+  ASSERT_EQ(zim::getClusterCacheCurrentSize(), 0);
 
 #define TEST_BROKEN_ZIM_NAME(ZIMNAME, EXPECTED)                \
 for(auto& testfile: getDataFilePath(ZIMNAME)) {EXPECT_BROKEN_ZIMFILE(testfile.path, EXPECTED)}