From 1514661c26596738f8aef4ec9327414f871a954f Mon Sep 17 00:00:00 2001
From: Matthieu Gautier <mgautier@kymeria.fr>
Date: Tue, 24 May 2022 17:19:39 +0200
Subject: [PATCH] Protect search from multi threading race condition.

libzim's search is not thread safe (mainly because xapian is not).
So we must protect our search objects from multi thread calls.

The best way to do this is to associate a mutex to the `zim::Searcher`
and lock the searcher each time we access object derivated from the
searcher (search, results, iterator, ...)
---
 include/library.h             | 20 ++++++++++++++++++--
 src/library.cpp               |  6 +++---
 src/server/internalServer.cpp |  4 +++-
 3 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/library.h b/include/library.h
index 0d5d19f0f..ef43f2a7e 100644
--- a/include/library.h
+++ b/include/library.h
@@ -141,6 +141,22 @@ private: // functions
     bool accept(const Book& book) const;
 };
 
+
+class ZimSearcher : public zim::Searcher
+{
+  public:
+    explicit ZimSearcher(zim::Searcher&& searcher)
+      : zim::Searcher(searcher)
+    {}
+
+    std::unique_lock<std::mutex> getLock() {
+      return std::unique_lock<std::mutex>(m_mutex);
+    }
+    virtual ~ZimSearcher() = default;
+  private:
+    std::mutex m_mutex;
+};
+
 /**
  * A Library store several books.
  */
@@ -209,10 +225,10 @@ class Library
 
   DEPRECATED std::shared_ptr<Reader> getReaderById(const std::string& id);
   std::shared_ptr<zim::Archive> getArchiveById(const std::string& id);
-  std::shared_ptr<zim::Searcher> getSearcherById(const std::string& id) {
+  std::shared_ptr<ZimSearcher> getSearcherById(const std::string& id) {
     return getSearcherByIds(BookIdSet{id});
   }
-  std::shared_ptr<zim::Searcher> getSearcherByIds(const BookIdSet& ids);
+  std::shared_ptr<ZimSearcher> getSearcherByIds(const BookIdSet& ids);
 
   /**
    * Remove a book from the library.
diff --git a/src/library.cpp b/src/library.cpp
index d4b933c4d..ad97a0c38 100644
--- a/src/library.cpp
+++ b/src/library.cpp
@@ -93,7 +93,7 @@ struct Library::Impl
   std::map<std::string, Entry> m_books;
   using ArchiveCache = ConcurrentCache<std::string, std::shared_ptr<zim::Archive>>;
   std::unique_ptr<ArchiveCache> mp_archiveCache;
-  using SearcherCache = MultiKeyCache<std::string, std::shared_ptr<zim::Searcher>>;
+  using SearcherCache = MultiKeyCache<std::string, std::shared_ptr<ZimSearcher>>;
   std::unique_ptr<SearcherCache> mp_searcherCache;
   std::vector<kiwix::Bookmark> m_bookmarks;
   Xapian::WritableDatabase m_bookDB;
@@ -304,7 +304,7 @@ std::shared_ptr<zim::Archive> Library::getArchiveById(const std::string& id)
   }
 }
 
-std::shared_ptr<zim::Searcher> Library::getSearcherByIds(const BookIdSet& ids)
+std::shared_ptr<ZimSearcher> Library::getSearcherByIds(const BookIdSet& ids)
 {
   assert(!ids.empty());
   try {
@@ -318,7 +318,7 @@ std::shared_ptr<zim::Searcher> Library::getSearcherByIds(const BookIdSet& ids)
         }
         archives.push_back(*archive);
       }
-      return std::make_shared<zim::Searcher>(archives);
+      return std::make_shared<ZimSearcher>(zim::Searcher(archives));
     });
   } catch (std::invalid_argument&) {
     return nullptr;
diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp
index c4f6fd7cb..0a61f0ee4 100644
--- a/src/server/internalServer.cpp
+++ b/src/server/internalServer.cpp
@@ -704,11 +704,13 @@ std::unique_ptr<Response> InternalServer::handle_search(const RequestContext& re
 
     /* Make the search */
     // Try to get a search from the searchInfo, else build it
+    auto searcher = mp_library->getSearcherByIds(bookIds);
+    auto lock(searcher->getLock());
+
     std::shared_ptr<zim::Search> search;
     try {
       search = searchCache.getOrPut(searchInfo,
         [=](){
-          auto searcher = mp_library->getSearcherByIds(bookIds);
           return make_shared<zim::Search>(searcher->search(searchInfo.getZimQuery(m_verbose.load())));
         }
       );