From 49e99e7c224bf396fc4fa7d0fc428adb4aa3175c Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 18 Sep 2023 16:19:56 +0200 Subject: [PATCH 01/11] Remove dumpers from the public API. All those dumper were not used by any of our other projects. They are only used internally, either by `Library::writeToFile` or the server. --- include/meson.build | 4 ---- {include => src}/html_dumper.h | 0 {include => src}/library_dumper.h | 0 {include => src}/libxml_dumper.h | 0 {include => src}/opds_dumper.h | 0 5 files changed, 4 deletions(-) rename {include => src}/html_dumper.h (100%) rename {include => src}/library_dumper.h (100%) rename {include => src}/libxml_dumper.h (100%) rename {include => src}/opds_dumper.h (100%) diff --git a/include/meson.build b/include/meson.build index 405297a34..03e6f0465 100644 --- a/include/meson.build +++ b/include/meson.build @@ -4,10 +4,6 @@ headers = [ 'common.h', 'library.h', 'manager.h', - 'libxml_dumper.h', - 'opds_dumper.h', - 'library_dumper.h', - 'html_dumper.h', 'downloader.h', 'search_renderer.h', 'server.h', diff --git a/include/html_dumper.h b/src/html_dumper.h similarity index 100% rename from include/html_dumper.h rename to src/html_dumper.h diff --git a/include/library_dumper.h b/src/library_dumper.h similarity index 100% rename from include/library_dumper.h rename to src/library_dumper.h diff --git a/include/libxml_dumper.h b/src/libxml_dumper.h similarity index 100% rename from include/libxml_dumper.h rename to src/libxml_dumper.h diff --git a/include/opds_dumper.h b/src/opds_dumper.h similarity index 100% rename from include/opds_dumper.h rename to src/opds_dumper.h From c203e07ee917f459122e267ec6e8fecb2a3f9d3d Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 18 Sep 2023 17:16:12 +0200 Subject: [PATCH 02/11] Make the library creatable only within a shared_ptr. --- include/library.h | 9 ++- test/library.cpp | 106 ++++++++++++++++++------------------ test/manager.cpp | 26 ++++----- test/name_mapper.cpp | 28 +++++----- test/server_testing_tools.h | 12 ++-- 5 files changed, 96 insertions(+), 85 deletions(-) diff --git a/include/library.h b/include/library.h index 602cfb9fd..b9ca15cea 100644 --- a/include/library.h +++ b/include/library.h @@ -176,7 +176,7 @@ class ZimSearcher : public zim::Searcher /** * A Library store several books. */ -class Library +class Library: public std::enable_shared_from_this { // all data fields must be added in LibraryBase mutable std::mutex m_mutex; @@ -187,8 +187,13 @@ class Library typedef std::map AttributeCounts; typedef std::set BookIdSet; - public: + private: Library(); + + public: + [[nodiscard]] static std::shared_ptr create() { + return std::shared_ptr(new Library()); + } ~Library(); /** diff --git a/test/library.cpp b/test/library.cpp index 4fb4b7dd1..aa75eb80a 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -238,14 +238,14 @@ typedef std::vector Langs; TEST(LibraryOpdsImportTest, allInOne) { - kiwix::Library lib; - kiwix::Manager manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager(lib.get()); manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev"); - EXPECT_EQ(10U, lib.getBookCount(true, true)); + EXPECT_EQ(10U, lib->getBookCount(true, true)); { - const kiwix::Book& book1 = lib.getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); + const kiwix::Book& book1 = lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd"); EXPECT_EQ(book1.getTitle(), "Encyclopédie de la Tunisie"); EXPECT_EQ(book1.getName(), "wikipedia_fr_tunisie_novid_2018-10"); @@ -271,7 +271,7 @@ TEST(LibraryOpdsImportTest, allInOne) } { - const kiwix::Book& book2 = lib.getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); + const kiwix::Book& book2 = lib->getBookById("0189d9be-2fd0-b4b6-7300-20fab0b5cdc8"); EXPECT_EQ(book2.getTitle(), "TED talks - Business"); EXPECT_EQ(book2.getName(), ""); EXPECT_EQ(book2.getFlavour(), ""); @@ -301,8 +301,10 @@ class LibraryTest : public ::testing::Test { typedef kiwix::Library::BookIdCollection BookIdCollection; typedef std::vector TitleCollection; + LibraryTest(): lib(kiwix::Library::create()) {} + void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib.get()); manager.readOpds(sampleOpdsStream, "foo.urlHost"); manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } @@ -316,25 +318,25 @@ class LibraryTest : public ::testing::Test { TitleCollection ids2Titles(const BookIdCollection& ids) { TitleCollection titles; for ( const auto& bookId : ids ) { - titles.push_back(lib.getBookById(bookId).getTitle()); + titles.push_back(lib->getBookById(bookId).getTitle()); } std::sort(titles.begin(), titles.end()); return titles; } - kiwix::Library lib; + std::shared_ptr lib; }; TEST_F(LibraryTest, getBookMarksTest) { - auto bookId1 = lib.getBooksIds()[0]; - auto bookId2 = lib.getBooksIds()[1]; + auto bookId1 = lib->getBooksIds()[0]; + auto bookId2 = lib->getBooksIds()[1]; - lib.addBookmark(createBookmark(bookId1)); - lib.addBookmark(createBookmark("invalid-bookmark-id")); - lib.addBookmark(createBookmark(bookId2)); - auto onlyValidBookmarks = lib.getBookmarks(); - auto allBookmarks = lib.getBookmarks(false); + lib->addBookmark(createBookmark(bookId1)); + lib->addBookmark(createBookmark("invalid-bookmark-id")); + lib->addBookmark(createBookmark(bookId2)); + auto onlyValidBookmarks = lib->getBookmarks(); + auto allBookmarks = lib->getBookmarks(false); EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1); EXPECT_EQ(onlyValidBookmarks[1].getBookId(), bookId2); @@ -346,11 +348,11 @@ TEST_F(LibraryTest, getBookMarksTest) TEST_F(LibraryTest, sanityCheck) { - EXPECT_EQ(lib.getBookCount(true, true), 12U); - EXPECT_EQ(lib.getBooksLanguages(), + EXPECT_EQ(lib->getBookCount(true, true), 12U); + EXPECT_EQ(lib->getBooksLanguages(), std::vector({"deu", "eng", "fra", "ita", "spa"}) ); - EXPECT_EQ(lib.getBooksCreators(), std::vector({ + EXPECT_EQ(lib->getBooksCreators(), std::vector({ "Islam Stack Exchange", "Movies & TV Stack Exchange", "Mythology & Folklore Stack Exchange", @@ -361,7 +363,7 @@ TEST_F(LibraryTest, sanityCheck) "Wikipedia", "Wikiquote" })); - EXPECT_EQ(lib.getBooksPublishers(), std::vector({ + EXPECT_EQ(lib->getBooksPublishers(), std::vector({ "", "Kiwix", "Kiwix & Some Enthusiasts", @@ -371,22 +373,22 @@ TEST_F(LibraryTest, sanityCheck) TEST_F(LibraryTest, categoryHandling) { - EXPECT_EQ("", lib.getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd").getCategory()); - EXPECT_EQ("category_defined_via_tags_only", lib.getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2").getCategory()); - EXPECT_EQ("category_defined_via_category_element_only", lib.getBookById("0ea1cde6-441d-6c58-f2c7-21c2838e659f").getCategory()); - EXPECT_EQ("category_element_overrides_tags", lib.getBookById("1123e574-6eef-6d54-28fc-13e4caeae474").getCategory()); - EXPECT_EQ("category_element_overrides_tags", lib.getBookById("14829621-c490-c376-0792-9de558b57efa").getCategory()); + EXPECT_EQ("", lib->getBookById("0c45160e-f917-760a-9159-dfe3c53cdcdd").getCategory()); + EXPECT_EQ("category_defined_via_tags_only", lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2").getCategory()); + EXPECT_EQ("category_defined_via_category_element_only", lib->getBookById("0ea1cde6-441d-6c58-f2c7-21c2838e659f").getCategory()); + EXPECT_EQ("category_element_overrides_tags", lib->getBookById("1123e574-6eef-6d54-28fc-13e4caeae474").getCategory()); + EXPECT_EQ("category_element_overrides_tags", lib->getBookById("14829621-c490-c376-0792-9de558b57efa").getCategory()); } TEST_F(LibraryTest, emptyFilter) { - const auto bookIds = lib.filter(kiwix::Filter()); - EXPECT_EQ(bookIds, lib.getBooksIds()); + const auto bookIds = lib->filter(kiwix::Filter()); + EXPECT_EQ(bookIds, lib->getBooksIds()); } #define EXPECT_FILTER_RESULTS(f, ...) \ EXPECT_EQ( \ - ids2Titles(lib.filter(f)), \ + ids2Titles(lib->filter(f)), \ TitleCollection({ __VA_ARGS__ }) \ ) @@ -736,33 +738,33 @@ TEST_F(LibraryTest, filterByMultipleCriteria) TEST_F(LibraryTest, getBookByPath) { - kiwix::Book book = lib.getBookById(lib.getBooksIds()[0]); + kiwix::Book book = lib->getBookById(lib->getBooksIds()[0]); #ifdef _WIN32 auto path = "C:\\some\\abs\\path.zim"; #else auto path = "/some/abs/path.zim"; #endif book.setPath(path); - lib.addBook(book); - EXPECT_EQ(lib.getBookByPath(path).getId(), book.getId()); - EXPECT_THROW(lib.getBookByPath("non/existant/path.zim"), std::out_of_range); + lib->addBook(book); + EXPECT_EQ(lib->getBookByPath(path).getId(), book.getId()); + EXPECT_THROW(lib->getBookByPath("non/existant/path.zim"), std::out_of_range); } TEST_F(LibraryTest, removeBookByIdRemovesTheBook) { - const auto initialBookCount = lib.getBookCount(true, true); + const auto initialBookCount = lib->getBookCount(true, true); ASSERT_GT(initialBookCount, 0U); - EXPECT_NO_THROW(lib.getBookById("raycharles")); - lib.removeBookById("raycharles"); - EXPECT_EQ(initialBookCount - 1, lib.getBookCount(true, true)); - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); + EXPECT_NO_THROW(lib->getBookById("raycharles")); + lib->removeBookById("raycharles"); + EXPECT_EQ(initialBookCount - 1, lib->getBookCount(true, true)); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBookByIdDropsTheReader) { - EXPECT_NE(nullptr, lib.getArchiveById("raycharles")); - lib.removeBookById("raycharles"); - EXPECT_THROW(lib.getArchiveById("raycharles"), std::out_of_range); + EXPECT_NE(nullptr, lib->getArchiveById("raycharles")); + lib->removeBookById("raycharles"); + EXPECT_THROW(lib->getArchiveById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBookByIdUpdatesTheSearchDB) @@ -770,17 +772,17 @@ TEST_F(LibraryTest, removeBookByIdUpdatesTheSearchDB) kiwix::Filter f; f.local(true).valid(true).query(R"(title:"ray charles")", false); - EXPECT_NO_THROW(lib.getBookById("raycharles")); - EXPECT_EQ(1U, lib.filter(f).size()); + EXPECT_NO_THROW(lib->getBookById("raycharles")); + EXPECT_EQ(1U, lib->filter(f).size()); - lib.removeBookById("raycharles"); + lib->removeBookById("raycharles"); - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); - EXPECT_EQ(0U, lib.filter(f).size()); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); + EXPECT_EQ(0U, lib->filter(f).size()); // make sure that Library::filter() doesn't add an empty book with // an id surviving in the search DB - EXPECT_THROW(lib.getBookById("raycharles"), std::out_of_range); + EXPECT_THROW(lib->getBookById("raycharles"), std::out_of_range); }; TEST_F(LibraryTest, removeBooksNotUpdatedSince) @@ -800,18 +802,18 @@ TEST_F(LibraryTest, removeBooksNotUpdatedSince) "Wikiquote" ); - const uint64_t rev = lib.getRevision(); - for ( const auto& id : lib.filter(kiwix::Filter().query("exchange")) ) { - lib.addBook(lib.getBookByIdThreadSafe(id)); + const uint64_t rev = lib->getRevision(); + for ( const auto& id : lib->filter(kiwix::Filter().query("exchange")) ) { + lib->addBook(lib->getBookByIdThreadSafe(id)); } - EXPECT_GT(lib.getRevision(), rev); + EXPECT_GT(lib->getRevision(), rev); - const uint64_t rev2 = lib.getRevision(); + const uint64_t rev2 = lib->getRevision(); - EXPECT_EQ(9u, lib.removeBooksNotUpdatedSince(rev)); + EXPECT_EQ(9u, lib->removeBooksNotUpdatedSince(rev)); - EXPECT_GT(lib.getRevision(), rev2); + EXPECT_GT(lib->getRevision(), rev2); EXPECT_FILTER_RESULTS(kiwix::Filter(), "Islam Stack Exchange", diff --git a/test/manager.cpp b/test/manager.cpp index e2b600644..21a492005 100644 --- a/test/manager.cpp +++ b/test/manager.cpp @@ -8,18 +8,18 @@ TEST(ManagerTest, addBookFromPathAndGetIdTest) { - kiwix::Library lib; - kiwix::Manager manager = kiwix::Manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager = kiwix::Manager(lib.get()); auto bookId = manager.addBookFromPathAndGetId("./test/example.zim"); ASSERT_NE(bookId, ""); - kiwix::Book book = lib.getBookById(bookId); + kiwix::Book book = lib->getBookById(bookId); EXPECT_EQ(book.getPath(), kiwix::computeAbsolutePath("", "./test/example.zim")); const std::string pathToSave = "./pathToSave"; const std::string url = "url"; bookId = manager.addBookFromPathAndGetId("./test/example.zim", pathToSave, url, true); - book = lib.getBookById(bookId); + book = lib->getBookById(bookId); auto savedPath = kiwix::computeAbsolutePath(kiwix::removeLastPathElement(manager.writableLibraryPath), pathToSave); EXPECT_EQ(book.getPath(), savedPath); EXPECT_EQ(book.getUrl(), url); @@ -48,11 +48,11 @@ const char sampleLibraryXML[] = R"( TEST(ManagerTest, readXml) { - kiwix::Library lib; - kiwix::Manager manager = kiwix::Manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager = kiwix::Manager(lib.get()); EXPECT_EQ(true, manager.readXml(sampleLibraryXML, true, "/data/lib.xml", true)); - kiwix::Book book = lib.getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2"); + kiwix::Book book = lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2"); EXPECT_EQ("/data/zimfiles/unittest.zim", book.getPath()); EXPECT_EQ("https://example.com/zimfiles/unittest.zim", book.getUrl()); EXPECT_EQ("Unit Test", book.getTitle()); @@ -70,24 +70,24 @@ TEST(ManagerTest, readXml) TEST(Manager, reload) { - kiwix::Library lib; - kiwix::Manager manager(&lib); + auto lib = kiwix::Library::create(); + kiwix::Manager manager(lib.get()); manager.reload({ "./test/library.xml" }); - EXPECT_EQ(lib.getBooksIds(), (kiwix::Library::BookIdCollection{ + EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{ "charlesray", "raycharles", "raycharles_uncategorized" })); - lib.removeBookById("raycharles"); - EXPECT_EQ(lib.getBooksIds(), (kiwix::Library::BookIdCollection{ + lib->removeBookById("raycharles"); + EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{ "charlesray", "raycharles_uncategorized" })); manager.reload({ "./test/library.xml" }); - EXPECT_EQ(lib.getBooksIds(), kiwix::Library::BookIdCollection({ + EXPECT_EQ(lib->getBooksIds(), kiwix::Library::BookIdCollection({ "charlesray", "raycharles", "raycharles_uncategorized" diff --git a/test/name_mapper.cpp b/test/name_mapper.cpp index 0bc60254a..cb7af2686 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -18,18 +18,20 @@ const char libraryXML[] = R"( )"; class NameMapperTest : public ::testing::Test { + public: + NameMapperTest(): lib(kiwix::Library::create()) {} protected: void SetUp() override { - kiwix::Manager manager(&lib); + kiwix::Manager manager(lib.get()); manager.readXml(libraryXML, false, "./library.xml", true); - for ( const std::string& id : lib.getBooksIds() ) { - kiwix::Book bookCopy = lib.getBookById(id); + for ( const std::string& id : lib->getBooksIds() ) { + kiwix::Book bookCopy = lib->getBookById(id); bookCopy.setPathValid(true); - lib.addBook(bookCopy); + lib->addBook(bookCopy); } } - kiwix::Library lib; + std::shared_ptr lib; }; class CapturedStderr @@ -73,13 +75,13 @@ void checkUnaliasedEntriesInNameMapper(const kiwix::NameMapper& nm) TEST_F(NameMapperTest, HumanReadableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::HumanReadableNameMapper nm(lib, false); + kiwix::HumanReadableNameMapper nm(*lib, false); EXPECT_EQ("", std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); EXPECT_EQ("zero_four_2021-10", nm.getNameForId("04-2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four_2021-10")); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); @@ -88,7 +90,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithoutAliases) TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::HumanReadableNameMapper nm(lib, true); + kiwix::HumanReadableNameMapper nm(*lib, true); EXPECT_EQ( "Path collision: /data/zero_four_2021-10.zim and" " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." @@ -99,7 +101,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) checkUnaliasedEntriesInNameMapper(nm); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); EXPECT_EQ("zero_four_2021-10", nm.getNameForId("04-2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four_2021-10")); EXPECT_EQ("04-2021-10", nm.getIdForName("zero_four")); @@ -108,13 +110,13 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, false); + kiwix::UpdatableNameMapper nm(*lib, false); EXPECT_EQ("", std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); EXPECT_THROW(nm.getIdForName("zero_four"), std::out_of_range); - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); nm.update(); EXPECT_THROW(nm.getNameForId("04-2021-10"), std::out_of_range); EXPECT_THROW(nm.getIdForName("zero_four_2021-10"), std::out_of_range); @@ -124,7 +126,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(lib, true); + kiwix::UpdatableNameMapper nm(*lib, true); EXPECT_EQ( "Path collision: /data/zero_four_2021-10.zim and" " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." @@ -137,7 +139,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr nmUpdateStderror; - lib.removeBookById("04-2021-10"); + lib->removeBookById("04-2021-10"); nm.update(); EXPECT_EQ("", std::string(nmUpdateStderror)); } diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 777bb48e8..1974a0132 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -98,7 +98,7 @@ private: void run(int serverPort, std::string indexTemplateString = ""); private: // data - kiwix::Library library; + std::shared_ptr library; kiwix::Manager manager; std::unique_ptr nameMapper; std::unique_ptr server; @@ -107,7 +107,8 @@ private: // data }; ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, std::string libraryFilePath) -: manager(&this->library) +: library(kiwix::Library::create()) +, manager(this->library.get()) , cfg(_cfg) { if ( kiwix::isRelativePath(libraryFilePath) ) @@ -120,7 +121,8 @@ ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, const FilePathCollection& zimpaths, std::string indexTemplateString) -: manager(&this->library) +: library(kiwix::Library::create()) +, manager(this->library.get()) , cfg(_cfg) { for ( const auto& zimpath : zimpaths ) { @@ -136,9 +138,9 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) if (cfg.options & NO_NAME_MAPPER) { nameMapper.reset(new kiwix::IdNameMapper()); } else { - nameMapper.reset(new kiwix::HumanReadableNameMapper(library, false)); + nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); } - server.reset(new kiwix::Server(&library, nameMapper.get())); + server.reset(new kiwix::Server(library.get(), nameMapper.get())); server->setRoot(cfg.root); server->setAddress(address); server->setPort(serverPort); From 139b561253057390c9921fff580780e9ddca7a7c Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 25 Sep 2023 16:12:09 +0200 Subject: [PATCH 03/11] Make the `Manager` keep a `shared_ptr` instead of a raw Library reference. We want to be sure that `Library` actually exists when we modify it. While it is not a silver bullet (user can still create a shared_ptr on a raw pointer), making the `Manager` keep `shared_ptr` on the library help us a lot here. --- include/manager.h | 12 +++++------ src/manager.cpp | 40 ++++++++++++++----------------------- test/library.cpp | 4 ++-- test/manager.cpp | 6 +++--- test/name_mapper.cpp | 2 +- test/server_testing_tools.h | 4 ++-- 6 files changed, 29 insertions(+), 39 deletions(-) diff --git a/include/manager.h b/include/manager.h index d0addad70..9b13da39c 100644 --- a/include/manager.h +++ b/include/manager.h @@ -37,10 +37,10 @@ namespace kiwix class LibraryManipulator { public: // functions - explicit LibraryManipulator(Library* library); + explicit LibraryManipulator(std::shared_ptr library); virtual ~LibraryManipulator(); - Library& getLibrary() const { return library; } + std::shared_ptr getLibrary() const { return library; } bool addBookToLibrary(const Book& book); void addBookmarkToLibrary(const Bookmark& bookmark); @@ -52,7 +52,7 @@ class LibraryManipulator virtual void booksWereRemovedFromLibrary(); private: // data - kiwix::Library& library; + std::shared_ptr library; }; /** @@ -64,8 +64,8 @@ class Manager typedef std::vector Paths; public: // functions - explicit Manager(LibraryManipulator* manipulator); - explicit Manager(Library* library); + explicit Manager(LibraryManipulator manipulator); + explicit Manager(std::shared_ptr library); /** * Read a `library.xml` and add book in the file to the library. @@ -163,7 +163,7 @@ class Manager uint64_t m_itemsPerPage = 0; protected: - std::shared_ptr manipulator; + kiwix::LibraryManipulator manipulator; bool readBookFromPath(const std::string& path, Book* book); bool parseXmlDom(const pugi::xml_document& doc, diff --git a/src/manager.cpp b/src/manager.cpp index 55d9eff70..9a4f9b28c 100644 --- a/src/manager.cpp +++ b/src/manager.cpp @@ -27,22 +27,12 @@ namespace kiwix { -namespace -{ - -struct NoDelete -{ - template void operator()(T*) {} -}; - -} // unnamed namespace - //////////////////////////////////////////////////////////////////////////////// // LibraryManipulator //////////////////////////////////////////////////////////////////////////////// -LibraryManipulator::LibraryManipulator(Library* library) - : library(*library) +LibraryManipulator::LibraryManipulator(std::shared_ptr library) + : library(library) {} LibraryManipulator::~LibraryManipulator() @@ -50,7 +40,7 @@ LibraryManipulator::~LibraryManipulator() bool LibraryManipulator::addBookToLibrary(const Book& book) { - const auto ret = library.addBook(book); + const auto ret = library->addBook(book); if ( ret ) { bookWasAddedToLibrary(book); } @@ -59,13 +49,13 @@ bool LibraryManipulator::addBookToLibrary(const Book& book) void LibraryManipulator::addBookmarkToLibrary(const Bookmark& bookmark) { - library.addBookmark(bookmark); + library->addBookmark(bookmark); bookmarkWasAddedToLibrary(bookmark); } uint32_t LibraryManipulator::removeBooksNotUpdatedSince(Library::Revision rev) { - const auto n = library.removeBooksNotUpdatedSince(rev); + const auto n = library->removeBooksNotUpdatedSince(rev); if ( n != 0 ) { booksWereRemovedFromLibrary(); } @@ -89,15 +79,15 @@ void LibraryManipulator::booksWereRemovedFromLibrary() //////////////////////////////////////////////////////////////////////////////// /* Constructor */ -Manager::Manager(LibraryManipulator* manipulator): +Manager::Manager(LibraryManipulator manipulator): writableLibraryPath(""), - manipulator(manipulator, NoDelete()) + manipulator(manipulator) { } -Manager::Manager(Library* library) : +Manager::Manager(std::shared_ptr library) : writableLibraryPath(""), - manipulator(new LibraryManipulator(library)) + manipulator(LibraryManipulator(library)) { } @@ -121,7 +111,7 @@ bool Manager::parseXmlDom(const pugi::xml_document& doc, if (!trustLibrary && !book.getPath().empty()) { this->readBookFromPath(book.getPath(), &book); } - manipulator->addBookToLibrary(book); + manipulator.addBookToLibrary(book); } return true; @@ -166,7 +156,7 @@ bool Manager::parseOpdsDom(const pugi::xml_document& doc, const std::string& url book.updateFromOpds(entryNode, urlHost); /* Update the book properties with the new importer */ - manipulator->addBookToLibrary(book); + manipulator.addBookToLibrary(book); } return true; @@ -241,7 +231,7 @@ std::string Manager::addBookFromPathAndGetId(const std::string& pathToOpen, || (!book.getTitle().empty() && !book.getLanguages().empty() && !book.getDate().empty())) { book.setUrl(url); - manipulator->addBookToLibrary(book); + manipulator.addBookToLibrary(book); return book.getId(); } } @@ -296,7 +286,7 @@ bool Manager::readBookmarkFile(const std::string& path) bookmark.updateFromXml(node); - manipulator->addBookmarkToLibrary(bookmark); + manipulator.addBookmarkToLibrary(bookmark); } return true; @@ -304,7 +294,7 @@ bool Manager::readBookmarkFile(const std::string& path) void Manager::reload(const Paths& paths) { - const auto libRevision = manipulator->getLibrary().getRevision(); + const auto libRevision = manipulator.getLibrary()->getRevision(); for (std::string path : paths) { if (!path.empty()) { if ( kiwix::isRelativePath(path) ) @@ -316,7 +306,7 @@ void Manager::reload(const Paths& paths) } } - manipulator->removeBooksNotUpdatedSince(libRevision); + manipulator.removeBooksNotUpdatedSince(libRevision); } } diff --git a/test/library.cpp b/test/library.cpp index aa75eb80a..30a77fcff 100644 --- a/test/library.cpp +++ b/test/library.cpp @@ -239,7 +239,7 @@ typedef std::vector Langs; TEST(LibraryOpdsImportTest, allInOne) { auto lib = kiwix::Library::create(); - kiwix::Manager manager(lib.get()); + kiwix::Manager manager(lib); manager.readOpds(sampleOpdsStream, "library-opds-import.unittests.dev"); EXPECT_EQ(10U, lib->getBookCount(true, true)); @@ -304,7 +304,7 @@ class LibraryTest : public ::testing::Test { LibraryTest(): lib(kiwix::Library::create()) {} void SetUp() override { - kiwix::Manager manager(lib.get()); + kiwix::Manager manager(lib); manager.readOpds(sampleOpdsStream, "foo.urlHost"); manager.readXml(sampleLibraryXML, false, "./test/library.xml", true); } diff --git a/test/manager.cpp b/test/manager.cpp index 21a492005..47f1edf86 100644 --- a/test/manager.cpp +++ b/test/manager.cpp @@ -9,7 +9,7 @@ TEST(ManagerTest, addBookFromPathAndGetIdTest) { auto lib = kiwix::Library::create(); - kiwix::Manager manager = kiwix::Manager(lib.get()); + kiwix::Manager manager = kiwix::Manager(lib); auto bookId = manager.addBookFromPathAndGetId("./test/example.zim"); ASSERT_NE(bookId, ""); @@ -49,7 +49,7 @@ const char sampleLibraryXML[] = R"( TEST(ManagerTest, readXml) { auto lib = kiwix::Library::create(); - kiwix::Manager manager = kiwix::Manager(lib.get()); + kiwix::Manager manager = kiwix::Manager(lib); EXPECT_EQ(true, manager.readXml(sampleLibraryXML, true, "/data/lib.xml", true)); kiwix::Book book = lib->getBookById("0d0bcd57-d3f6-cb22-44cc-a723ccb4e1b2"); @@ -71,7 +71,7 @@ TEST(ManagerTest, readXml) TEST(Manager, reload) { auto lib = kiwix::Library::create(); - kiwix::Manager manager(lib.get()); + kiwix::Manager manager(lib); manager.reload({ "./test/library.xml" }); EXPECT_EQ(lib->getBooksIds(), (kiwix::Library::BookIdCollection{ diff --git a/test/name_mapper.cpp b/test/name_mapper.cpp index cb7af2686..208b81240 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -22,7 +22,7 @@ class NameMapperTest : public ::testing::Test { NameMapperTest(): lib(kiwix::Library::create()) {} protected: void SetUp() override { - kiwix::Manager manager(lib.get()); + kiwix::Manager manager(lib); manager.readXml(libraryXML, false, "./library.xml", true); for ( const std::string& id : lib->getBooksIds() ) { kiwix::Book bookCopy = lib->getBookById(id); diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 1974a0132..6a797e345 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -108,7 +108,7 @@ private: // data ZimFileServer::ZimFileServer(int serverPort, Cfg _cfg, std::string libraryFilePath) : library(kiwix::Library::create()) -, manager(this->library.get()) +, manager(this->library) , cfg(_cfg) { if ( kiwix::isRelativePath(libraryFilePath) ) @@ -122,7 +122,7 @@ ZimFileServer::ZimFileServer(int serverPort, const FilePathCollection& zimpaths, std::string indexTemplateString) : library(kiwix::Library::create()) -, manager(this->library.get()) +, manager(this->library) , cfg(_cfg) { for ( const auto& zimpath : zimpaths ) { From efcbf6ef1eeff714aa9545ac7c70af44771cba49 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 25 Sep 2023 16:13:52 +0200 Subject: [PATCH 04/11] Make the `UpdatableNameMapper` keep a `shared_ptr`. Same as `Manager`, we want to be sure that `Library` actually exists when we use it. --- include/name_mapper.h | 4 ++-- src/name_mapper.cpp | 4 ++-- test/name_mapper.cpp | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/name_mapper.h b/include/name_mapper.h index 4247c5c3c..63333515d 100644 --- a/include/name_mapper.h +++ b/include/name_mapper.h @@ -59,7 +59,7 @@ class HumanReadableNameMapper : public NameMapper { class UpdatableNameMapper : public NameMapper { typedef std::shared_ptr NameMapperHandle; public: - UpdatableNameMapper(Library& library, bool withAlias); + UpdatableNameMapper(std::shared_ptr library, bool withAlias); virtual std::string getNameForId(const std::string& id) const; virtual std::string getIdForName(const std::string& name) const; @@ -71,7 +71,7 @@ class UpdatableNameMapper : public NameMapper { private: mutable std::mutex mutex; - Library& library; + std::shared_ptr library; NameMapperHandle nameMapper; const bool withAlias; }; diff --git a/src/name_mapper.cpp b/src/name_mapper.cpp index dccf40c9b..f44227042 100644 --- a/src/name_mapper.cpp +++ b/src/name_mapper.cpp @@ -63,7 +63,7 @@ std::string HumanReadableNameMapper::getIdForName(const std::string& name) const // UpdatableNameMapper //////////////////////////////////////////////////////////////////////////////// -UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias) +UpdatableNameMapper::UpdatableNameMapper(std::shared_ptr lib, bool withAlias) : library(lib) , withAlias(withAlias) { @@ -72,7 +72,7 @@ UpdatableNameMapper::UpdatableNameMapper(Library& lib, bool withAlias) void UpdatableNameMapper::update() { - const auto newNameMapper = new HumanReadableNameMapper(library, withAlias); + const auto newNameMapper = new HumanReadableNameMapper(*library, withAlias); std::lock_guard lock(mutex); nameMapper.reset(newNameMapper); } diff --git a/test/name_mapper.cpp b/test/name_mapper.cpp index 208b81240..4396356e8 100644 --- a/test/name_mapper.cpp +++ b/test/name_mapper.cpp @@ -110,7 +110,7 @@ TEST_F(NameMapperTest, HumanReadableNameMapperWithAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(*lib, false); + kiwix::UpdatableNameMapper nm(lib, false); EXPECT_EQ("", std::string(stderror)); checkUnaliasedEntriesInNameMapper(nm); @@ -126,7 +126,7 @@ TEST_F(NameMapperTest, UpdatableNameMapperWithoutAliases) TEST_F(NameMapperTest, UpdatableNameMapperWithAliases) { CapturedStderr stderror; - kiwix::UpdatableNameMapper nm(*lib, true); + kiwix::UpdatableNameMapper nm(lib, true); EXPECT_EQ( "Path collision: /data/zero_four_2021-10.zim and" " /data/zero_four_2021-11.zim can't share the same URL path 'zero_four'." From a5557eeb255a957ab63a6884cf35d41161657581 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 25 Sep 2023 16:15:32 +0200 Subject: [PATCH 05/11] Make the `Server` keep a `shared_ptr` instead of a raw Library pointer. We want to be sure that `Library` actually exists when we use it. While it is not a silver bullet (user can still create a shared_ptr on a raw pointer), making the `Server` keep `shared_ptr` on the library help us a lot here. --- include/server.h | 4 ++-- src/server.cpp | 2 +- src/server/internalServer.cpp | 6 +++--- src/server/internalServer.h | 4 ++-- src/server/internalServer_catalog.cpp | 10 +++++----- test/server_testing_tools.h | 2 +- 6 files changed, 14 insertions(+), 14 deletions(-) diff --git a/include/server.h b/include/server.h index 0cd579c57..bd96ed664 100644 --- a/include/server.h +++ b/include/server.h @@ -36,7 +36,7 @@ namespace kiwix * * @param library The library to serve. */ - Server(Library* library, NameMapper* nameMapper=nullptr); + Server(std::shared_ptr library, NameMapper* nameMapper=nullptr); virtual ~Server(); @@ -66,7 +66,7 @@ namespace kiwix std::string getAddress(); protected: - Library* mp_library; + std::shared_ptr mp_library; NameMapper* mp_nameMapper; std::string m_root = ""; std::string m_addr = ""; diff --git a/src/server.cpp b/src/server.cpp index e9373417c..1c331a406 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -29,7 +29,7 @@ namespace kiwix { -Server::Server(Library* library, NameMapper* nameMapper) : +Server::Server(std::shared_ptr library, NameMapper* nameMapper) : mp_library(library), mp_nameMapper(nameMapper), mp_server(nullptr) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 544f7e31f..098f476c5 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -406,7 +406,7 @@ public: }; -InternalServer::InternalServer(Library* library, +InternalServer::InternalServer(std::shared_ptr library, NameMapper* nameMapper, std::string addr, int port, @@ -787,7 +787,7 @@ std::unique_ptr InternalServer::handle_no_js(const RequestContext& req { const auto url = request.get_url(); const auto urlParts = kiwix::split(url, "/", true, false); - HTMLDumper htmlDumper(mp_library, mp_nameMapper); + HTMLDumper htmlDumper(mp_library.get(), mp_nameMapper); htmlDumper.setRootLocation(m_root); htmlDumper.setLibraryId(getLibraryId()); auto userLang = request.get_user_language(); @@ -958,7 +958,7 @@ std::unique_ptr InternalServer::handle_search_request(const RequestCon const auto pageLength = getSearchPageSize(request); /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, mp_library, start, + SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, mp_library.get(), start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 867b324e0..149bda5a8 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -92,7 +92,7 @@ class OPDSDumper; class InternalServer { public: - InternalServer(Library* library, + InternalServer(std::shared_ptr library, NameMapper* nameMapper, std::string addr, int port, @@ -178,7 +178,7 @@ class InternalServer { int m_ipConnectionLimit; struct MHD_Daemon* mp_daemon; - Library* mp_library; + std::shared_ptr mp_library; NameMapper* mp_nameMapper; SearchCache searchCache; diff --git a/src/server/internalServer_catalog.cpp b/src/server/internalServer_catalog.cpp index a43d968e9..f4b8d096b 100644 --- a/src/server/internalServer_catalog.cpp +++ b/src/server/internalServer_catalog.cpp @@ -82,7 +82,7 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(mp_library, mp_nameMapper); + kiwix::OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); std::vector bookIdsToDump; @@ -164,7 +164,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); const auto bookIds = search_catalog(request, opdsDumper); @@ -185,7 +185,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const + urlNotFoundMsg; } - OPDSDumper opdsDumper(mp_library, mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); @@ -198,7 +198,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); return ContentResponse::build( @@ -210,7 +210,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library, mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); return ContentResponse::build( diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 6a797e345..575b0841d 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -140,7 +140,7 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) } else { nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); } - server.reset(new kiwix::Server(library.get(), nameMapper.get())); + server.reset(new kiwix::Server(library, nameMapper.get())); server->setRoot(cfg.root); server->setAddress(address); server->setPort(serverPort); From 1316dec37cf038aa5b88d561f132584e5e81c535 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 25 Sep 2023 16:44:28 +0200 Subject: [PATCH 06/11] Make the `Server` keep a `shared_ptr` instead of a raw NameMapper pointer. Same as for `Library`, we want to be sure that the `NameMapper` actually exists when the server is using it. --- include/server.h | 4 ++-- src/server.cpp | 2 +- src/server/internalServer.cpp | 13 +++++++++---- src/server/internalServer.h | 4 ++-- src/server/internalServer_catalog.cpp | 10 +++++----- test/server_testing_tools.h | 4 ++-- 6 files changed, 21 insertions(+), 16 deletions(-) diff --git a/include/server.h b/include/server.h index bd96ed664..d1981c8c2 100644 --- a/include/server.h +++ b/include/server.h @@ -36,7 +36,7 @@ namespace kiwix * * @param library The library to serve. */ - Server(std::shared_ptr library, NameMapper* nameMapper=nullptr); + Server(std::shared_ptr library, std::shared_ptr nameMapper=nullptr); virtual ~Server(); @@ -67,7 +67,7 @@ namespace kiwix protected: std::shared_ptr mp_library; - NameMapper* mp_nameMapper; + std::shared_ptr mp_nameMapper; std::string m_root = ""; std::string m_addr = ""; std::string m_indexTemplateString = ""; diff --git a/src/server.cpp b/src/server.cpp index 1c331a406..2e258245f 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -29,7 +29,7 @@ namespace kiwix { -Server::Server(std::shared_ptr library, NameMapper* nameMapper) : +Server::Server(std::shared_ptr library, std::shared_ptr nameMapper) : mp_library(library), mp_nameMapper(nameMapper), mp_server(nullptr) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 098f476c5..11bdd0971 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -254,6 +254,11 @@ get_matching_if_none_match_etag(const RequestContext& r, const std::string& etag } } +struct NoDelete +{ + template void operator()(T*) {} +}; + } // unnamed namespace std::pair InternalServer::selectBooks(const RequestContext& request) const @@ -407,7 +412,7 @@ public: InternalServer::InternalServer(std::shared_ptr library, - NameMapper* nameMapper, + std::shared_ptr nameMapper, std::string addr, int port, std::string root, @@ -433,7 +438,7 @@ InternalServer::InternalServer(std::shared_ptr library, m_ipConnectionLimit(ipConnectionLimit), mp_daemon(nullptr), mp_library(library), - mp_nameMapper(nameMapper ? nameMapper : &defaultNameMapper), + mp_nameMapper(nameMapper ? nameMapper : std::shared_ptr(&defaultNameMapper, NoDelete())), searchCache(getEnvVar("KIWIX_SEARCH_CACHE_SIZE", DEFAULT_CACHE_SIZE)), suggestionSearcherCache(getEnvVar("KIWIX_SUGGESTION_SEARCHER_CACHE_SIZE", std::max((unsigned int) (mp_library->getBookCount(true, true)*0.1), 1U))), m_customizedResources(new CustomizedResources) @@ -787,7 +792,7 @@ std::unique_ptr InternalServer::handle_no_js(const RequestContext& req { const auto url = request.get_url(); const auto urlParts = kiwix::split(url, "/", true, false); - HTMLDumper htmlDumper(mp_library.get(), mp_nameMapper); + HTMLDumper htmlDumper(mp_library.get(), mp_nameMapper.get()); htmlDumper.setRootLocation(m_root); htmlDumper.setLibraryId(getLibraryId()); auto userLang = request.get_user_language(); @@ -958,7 +963,7 @@ std::unique_ptr InternalServer::handle_search_request(const RequestCon const auto pageLength = getSearchPageSize(request); /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper, mp_library.get(), start, + SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper.get(), mp_library.get(), start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); diff --git a/src/server/internalServer.h b/src/server/internalServer.h index 149bda5a8..e11c690b5 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -93,7 +93,7 @@ class OPDSDumper; class InternalServer { public: InternalServer(std::shared_ptr library, - NameMapper* nameMapper, + std::shared_ptr nameMapper, std::string addr, int port, std::string root, @@ -179,7 +179,7 @@ class InternalServer { struct MHD_Daemon* mp_daemon; std::shared_ptr mp_library; - NameMapper* mp_nameMapper; + std::shared_ptr mp_nameMapper; SearchCache searchCache; SuggestionSearcherCache suggestionSearcherCache; diff --git a/src/server/internalServer_catalog.cpp b/src/server/internalServer_catalog.cpp index f4b8d096b..fa50450a9 100644 --- a/src/server/internalServer_catalog.cpp +++ b/src/server/internalServer_catalog.cpp @@ -82,7 +82,7 @@ std::unique_ptr InternalServer::handle_catalog(const RequestContext& r } zim::Uuid uuid; - kiwix::OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); + kiwix::OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get()); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); std::vector bookIdsToDump; @@ -164,7 +164,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_root(const RequestCo std::unique_ptr InternalServer::handle_catalog_v2_entries(const RequestContext& request, bool partial) { - OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get()); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); const auto bookIds = search_catalog(request, opdsDumper); @@ -185,7 +185,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const + urlNotFoundMsg; } - OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get()); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); const auto opdsFeed = opdsDumper.dumpOPDSCompleteEntry(entryId); @@ -198,7 +198,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_complete_entry(const std::unique_ptr InternalServer::handle_catalog_v2_categories(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get()); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); return ContentResponse::build( @@ -210,7 +210,7 @@ std::unique_ptr InternalServer::handle_catalog_v2_categories(const Req std::unique_ptr InternalServer::handle_catalog_v2_languages(const RequestContext& request) { - OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper); + OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get()); opdsDumper.setRootLocation(m_root); opdsDumper.setLibraryId(getLibraryId()); return ContentResponse::build( diff --git a/test/server_testing_tools.h b/test/server_testing_tools.h index 575b0841d..3ea10ab57 100644 --- a/test/server_testing_tools.h +++ b/test/server_testing_tools.h @@ -100,7 +100,7 @@ private: private: // data std::shared_ptr library; kiwix::Manager manager; - std::unique_ptr nameMapper; + std::shared_ptr nameMapper; std::unique_ptr server; std::unique_ptr client; const Cfg cfg; @@ -140,7 +140,7 @@ void ZimFileServer::run(int serverPort, std::string indexTemplateString) } else { nameMapper.reset(new kiwix::HumanReadableNameMapper(*library, false)); } - server.reset(new kiwix::Server(library, nameMapper.get())); + server.reset(new kiwix::Server(library, nameMapper)); server->setRoot(cfg.root); server->setAddress(address); server->setPort(serverPort); From ead1474ead237d820d9b7bb12ea973635817fcc7 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 25 Sep 2023 16:17:54 +0200 Subject: [PATCH 07/11] Make `SearchRendered` taking a const pointer. --- include/search_renderer.h | 6 +++--- src/search_renderer.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/search_renderer.h b/include/search_renderer.h index 0190ee5f0..d26d571a9 100644 --- a/include/search_renderer.h +++ b/include/search_renderer.h @@ -46,7 +46,7 @@ class SearchRenderer * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, + SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, unsigned int start, unsigned int estimatedResultCount); /** @@ -58,7 +58,7 @@ class SearchRenderer * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library, + SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, Library* library, unsigned int start, unsigned int estimatedResultCount); ~SearchRenderer(); @@ -106,7 +106,7 @@ class SearchRenderer protected: std::string beautifyInteger(const unsigned int number); zim::SearchResultSet m_srs; - NameMapper* mp_nameMapper; + const NameMapper* mp_nameMapper; Library* mp_library; std::string searchBookQuery; std::string searchPattern; diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index c47b93368..ab49a8c8c 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -36,12 +36,12 @@ namespace kiwix { /* Constructor */ -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, unsigned int start, unsigned int estimatedResultCount) : SearchRenderer(srs, mapper, nullptr, start, estimatedResultCount) {} -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, NameMapper* mapper, Library* library, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, Library* library, unsigned int start, unsigned int estimatedResultCount) : m_srs(srs), mp_nameMapper(mapper), From f8e7c3d4762b15264e00a0fb8b2db47d0fef5281 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 22 Aug 2023 15:52:54 +0200 Subject: [PATCH 08/11] Move the `Library` mutex in `Library::Impl`. The why of this mutex is in `Library` is a bit complex. It has been introduced in c2927ce when there was only `Library` and no `std::unique_ptr`. As introducing the mutex imply implementing the move constructor, we have split all data in `LibraryBase` (and keep a default move constructor here) and add the mutex in `Library` (and implement a simple move constructor). Later, in 090c2fd, we have move the `LibraryBase` to `Library::Impl` (which should have been `Library::Data`). So at the end, `Library::Impl` is never moved. We can move the `mutex` in it and still simply implement move constructor for `Library`. --- include/library.h | 3 --- src/library.cpp | 42 ++++++++++++++++++++---------------------- 2 files changed, 20 insertions(+), 25 deletions(-) diff --git a/include/library.h b/include/library.h index b9ca15cea..f617ce1d8 100644 --- a/include/library.h +++ b/include/library.h @@ -178,9 +178,6 @@ class ZimSearcher : public zim::Searcher */ class Library: public std::enable_shared_from_this { - // all data fields must be added in LibraryBase - mutable std::mutex m_mutex; - public: typedef uint64_t Revision; typedef std::vector BookIdCollection; diff --git a/src/library.cpp b/src/library.cpp index 6ee51602f..ba05814d1 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -88,6 +88,7 @@ struct Library::Impl Library::Revision lastUpdatedRevision = 0; }; + mutable std::mutex m_mutex; Library::Revision m_revision; std::map m_books; using ArchiveCache = ConcurrentCache>; @@ -102,8 +103,8 @@ struct Library::Impl Impl(); ~Impl(); - Impl(Impl&& ); - Impl& operator=(Impl&& ); + Impl(Impl&& ) = delete; + Impl& operator=(Impl&& ) = delete; }; Library::Impl::Impl() @@ -117,9 +118,6 @@ Library::Impl::~Impl() { } -Library::Impl::Impl(Library::Impl&& ) = default; -Library::Impl& Library::Impl::operator=(Library::Impl&& ) = default; - unsigned int Library::Impl::getBookCount(const bool localBooks, const bool remoteBooks) const { @@ -156,7 +154,7 @@ Library::~Library() = default; bool Library::addBook(const Book& book) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); ++mp_impl->m_revision; /* Try to find it */ updateBookDB(book); @@ -187,13 +185,13 @@ bool Library::addBook(const Book& book) void Library::addBookmark(const Bookmark& bookmark) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); mp_impl->m_bookmarks.push_back(bookmark); } bool Library::removeBookmark(const std::string& zimId, const std::string& url) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); for(auto it=mp_impl->m_bookmarks.begin(); it!=mp_impl->m_bookmarks.end(); it++) { if (it->getBookId() == zimId && it->getUrl() == url) { mp_impl->m_bookmarks.erase(it); @@ -212,7 +210,7 @@ void Library::dropCache(const std::string& id) bool Library::removeBookById(const std::string& id) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); mp_impl->m_bookDB.delete_document("Q" + id); dropCache(id); // We do not change the cache size here @@ -230,7 +228,7 @@ bool Library::removeBookById(const std::string& id) Library::Revision Library::getRevision() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); return mp_impl->m_revision; } @@ -238,7 +236,7 @@ uint32_t Library::removeBooksNotUpdatedSince(Revision libraryRevision) { BookIdCollection booksToRemove; { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); for ( const auto& entry : mp_impl->m_books) { if ( entry.second.lastUpdatedRevision <= libraryRevision ) { booksToRemove.push_back(entry.first); @@ -263,7 +261,7 @@ const Book& Library::getBookById(const std::string& id) const Book Library::getBookByIdThreadSafe(const std::string& id) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); return getBookById(id); } @@ -321,7 +319,7 @@ std::shared_ptr Library::getSearcherByIds(const BookIdSet& ids) unsigned int Library::getBookCount(const bool localBooks, const bool remoteBooks) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); return mp_impl->getBookCount(localBooks, remoteBooks); } @@ -334,7 +332,7 @@ bool Library::writeToFile(const std::string& path) const dumper.setBaseDir(baseDir); std::string xml; { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); xml = dumper.dumpLibXMLContent(allBookIds); }; return writeTextFile(path, xml); @@ -350,7 +348,7 @@ bool Library::writeBookmarksToFile(const std::string& path) const Library::AttributeCounts Library::getBookAttributeCounts(BookStrPropMemFn p) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); AttributeCounts propValueCounts; for (const auto& pair: mp_impl->m_books) { @@ -382,7 +380,7 @@ std::vector Library::getBooksLanguages() const Library::AttributeCounts Library::getBooksLanguagesWithCounts() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); AttributeCounts langsWithCounts; for (const auto& pair: mp_impl->m_books) { @@ -398,7 +396,7 @@ Library::AttributeCounts Library::getBooksLanguagesWithCounts() const std::vector Library::getBooksCategories() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); std::set categories; for (const auto& pair: mp_impl->m_books) { @@ -429,7 +427,7 @@ const std::vector Library::getBookmarks(bool onlyValidBookmarks } std::vector validBookmarks; auto booksId = getBooksIds(); - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); for(auto& bookmark:mp_impl->m_bookmarks) { if (std::find(booksId.begin(), booksId.end(), bookmark.getBookId()) != booksId.end()) { validBookmarks.push_back(bookmark); @@ -440,7 +438,7 @@ const std::vector Library::getBookmarks(bool onlyValidBookmarks Library::BookIdCollection Library::getBooksIds() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); BookIdCollection bookIds; for (auto& pair: mp_impl->m_books) { @@ -646,7 +644,7 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const BookIdCollection bookIds; - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); Xapian::Enquire enquire(mp_impl->m_bookDB); enquire.set_query(query); const auto results = enquire.get_mset(0, mp_impl->m_books.size()); @@ -661,7 +659,7 @@ Library::BookIdCollection Library::filter(const Filter& filter) const { BookIdCollection result; const auto preliminaryResult = filterViaBookDB(filter); - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); for(auto id : preliminaryResult) { if(filter.accept(mp_impl->m_books.at(id))) { result.push_back(id); @@ -735,7 +733,7 @@ void Library::sort(BookIdCollection& bookIds, supportedListSortBy sort, bool asc // NOTE: for the entire duration of the sort. Will need to obtain (under a // NOTE: lock) the required atributes from the books once, and then the // NOTE: sorting will run on a copy of data without locking. - std::lock_guard lock(m_mutex); + std::lock_guard lock(mp_impl->m_mutex); switch(sort) { case TITLE: std::sort(bookIds.begin(), bookIds.end(), Comparator(this, ascending)); From 5292f06fffaff83f821a9f785ccc0ca869229ef0 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier <mgautier@kymeria.fr> Date: Wed, 23 Aug 2023 10:42:30 +0200 Subject: [PATCH 09/11] Move back `Library::Impl` in `Library`. As we now always use Library through a shared_ptr, we can make `Library` not copiable and not movable. So we can move back the data in `Library`, we don't care about move constructor. --- include/library.h | 30 +++++++-- src/library.cpp | 160 +++++++++++++++++----------------------------- 2 files changed, 84 insertions(+), 106 deletions(-) diff --git a/include/library.h b/include/library.h index f617ce1d8..795792b33 100644 --- a/include/library.h +++ b/include/library.h @@ -34,6 +34,10 @@ #define KIWIX_LIBRARY_VERSION "20110515" +namespace Xapian { +class WritableDatabase; +}; + namespace kiwix { @@ -173,6 +177,12 @@ class ZimSearcher : public zim::Searcher std::mutex m_mutex; }; +template<typename, typename> +class ConcurrentCache; + +template<typename, typename> +class MultiKeyCache; + /** * A Library store several books. */ @@ -197,9 +207,9 @@ class Library: public std::enable_shared_from_this<Library> * Library is not a copiable object. However it can be moved. */ Library(const Library& ) = delete; - Library(Library&& ); + Library(Library&& ) = delete; void operator=(const Library& ) = delete; - Library& operator=(Library&& ); + Library& operator=(Library&& ) = delete; /** * Add a book to the library. @@ -370,17 +380,29 @@ class Library: public std::enable_shared_from_this<Library> private: // types typedef const std::string& (Book::*BookStrPropMemFn)() const; - struct Impl; + struct Entry : Book + { + Library::Revision lastUpdatedRevision = 0; + }; private: // functions AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const; std::vector<std::string> getBookPropValueSet(BookStrPropMemFn p) const; BookIdCollection filterViaBookDB(const Filter& filter) const; + unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const; void updateBookDB(const Book& book); void dropCache(const std::string& bookId); private: //data - std::unique_ptr<Impl> mp_impl; + mutable std::mutex m_mutex; + Library::Revision m_revision; + 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<ZimSearcher>>; + std::unique_ptr<SearcherCache> mp_searcherCache; + std::vector<kiwix::Bookmark> m_bookmarks; + std::unique_ptr<Xapian::WritableDatabase> m_bookDB; }; } diff --git a/src/library.cpp b/src/library.cpp index ba05814d1..702bf8bdd 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -39,6 +39,8 @@ namespace kiwix { + + namespace { @@ -58,6 +60,8 @@ bool booksReferToTheSameArchive(const Book& book1, const Book& book2) && book1.getPath() == book2.getPath(); } +} // unnamed namespace + template<typename Key, typename Value> class MultiKeyCache: public ConcurrentCache<std::set<Key>, Value> { @@ -79,47 +83,8 @@ class MultiKeyCache: public ConcurrentCache<std::set<Key>, Value> } }; -} // unnamed namespace - -struct Library::Impl -{ - struct Entry : Book - { - Library::Revision lastUpdatedRevision = 0; - }; - - mutable std::mutex m_mutex; - Library::Revision m_revision; - 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<ZimSearcher>>; - std::unique_ptr<SearcherCache> mp_searcherCache; - std::vector<kiwix::Bookmark> m_bookmarks; - Xapian::WritableDatabase m_bookDB; - - unsigned int getBookCount(const bool localBooks, const bool remoteBooks) const; - - Impl(); - ~Impl(); - - Impl(Impl&& ) = delete; - Impl& operator=(Impl&& ) = delete; -}; - -Library::Impl::Impl() - : mp_archiveCache(new ArchiveCache(std::max(getEnvVar<int>("KIWIX_ARCHIVE_CACHE_SIZE", 1), 1))), - mp_searcherCache(new SearcherCache(std::max(getEnvVar<int>("KIWIX_SEARCHER_CACHE_SIZE", 1), 1))), - m_bookDB("", Xapian::DB_BACKEND_INMEMORY) -{ -} - -Library::Impl::~Impl() -{ -} - unsigned int -Library::Impl::getBookCount(const bool localBooks, const bool remoteBooks) const +Library::getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const { unsigned int result = 0; for (auto& pair: m_books) { @@ -134,50 +99,41 @@ Library::Impl::getBookCount(const bool localBooks, const bool remoteBooks) const /* Constructor */ Library::Library() - : mp_impl(new Library::Impl) + : mp_archiveCache(new ArchiveCache(std::max(getEnvVar<int>("KIWIX_ARCHIVE_CACHE_SIZE", 1), 1))), + mp_searcherCache(new SearcherCache(std::max(getEnvVar<int>("KIWIX_SEARCHER_CACHE_SIZE", 1), 1))), + m_bookDB(new Xapian::WritableDatabase("", Xapian::DB_BACKEND_INMEMORY)) { } -Library::Library(Library&& other) - : mp_impl(std::move(other.mp_impl)) -{ -} - -Library& Library::operator=(Library&& other) -{ - mp_impl = std::move(other.mp_impl); - return *this; -} - /* Destructor */ Library::~Library() = default; bool Library::addBook(const Book& book) { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - ++mp_impl->m_revision; + std::lock_guard<std::mutex> lock(m_mutex); + ++m_revision; /* Try to find it */ updateBookDB(book); try { - auto& oldbook = mp_impl->m_books.at(book.getId()); + auto& oldbook = m_books.at(book.getId()); if ( ! booksReferToTheSameArchive(oldbook, book) ) { dropCache(book.getId()); } oldbook.update(book); // XXX: This may have no effect if oldbook is readonly // XXX: Then m_bookDB will become out-of-sync with // XXX: the real contents of the library. - oldbook.lastUpdatedRevision = mp_impl->m_revision; + oldbook.lastUpdatedRevision = m_revision; return false; } catch (std::out_of_range&) { - auto& newEntry = mp_impl->m_books[book.getId()]; + auto& newEntry = m_books[book.getId()]; static_cast<Book&>(newEntry) = book; - newEntry.lastUpdatedRevision = mp_impl->m_revision; - size_t new_cache_size = static_cast<size_t>(std::ceil(mp_impl->getBookCount(true, true)*0.1)); + newEntry.lastUpdatedRevision = m_revision; + size_t new_cache_size = static_cast<size_t>(std::ceil(getBookCount_not_protected(true, true)*0.1)); if (getEnvVar<int>("KIWIX_ARCHIVE_CACHE_SIZE", -1) <= 0) { - mp_impl->mp_archiveCache->setMaxSize(new_cache_size); + mp_archiveCache->setMaxSize(new_cache_size); } if (getEnvVar<int>("KIWIX_SEARCHER_CACHE_SIZE", -1) <= 0) { - mp_impl->mp_searcherCache->setMaxSize(new_cache_size); + mp_searcherCache->setMaxSize(new_cache_size); } return true; } @@ -185,16 +141,16 @@ bool Library::addBook(const Book& book) void Library::addBookmark(const Bookmark& bookmark) { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - mp_impl->m_bookmarks.push_back(bookmark); + std::lock_guard<std::mutex> lock(m_mutex); + m_bookmarks.push_back(bookmark); } bool Library::removeBookmark(const std::string& zimId, const std::string& url) { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - for(auto it=mp_impl->m_bookmarks.begin(); it!=mp_impl->m_bookmarks.end(); it++) { + std::lock_guard<std::mutex> lock(m_mutex); + for(auto it=m_bookmarks.begin(); it!=m_bookmarks.end(); it++) { if (it->getBookId() == zimId && it->getUrl() == url) { - mp_impl->m_bookmarks.erase(it); + m_bookmarks.erase(it); return true; } } @@ -204,14 +160,14 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) void Library::dropCache(const std::string& id) { - mp_impl->mp_archiveCache->drop(id); - mp_impl->mp_searcherCache->drop(id); + mp_archiveCache->drop(id); + mp_searcherCache->drop(id); } bool Library::removeBookById(const std::string& id) { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - mp_impl->m_bookDB.delete_document("Q" + id); + std::lock_guard<std::mutex> lock(m_mutex); + m_bookDB->delete_document("Q" + id); dropCache(id); // We do not change the cache size here // Most of the time, the book is remove in case of library refresh, it is @@ -219,25 +175,25 @@ bool Library::removeBookById(const std::string& id) // Having a too big cache is not a problem here (or it would have been before) // (And setMaxSize doesn't actually reduce the cache size, extra cached items // will be removed in put or getOrPut). - const bool bookWasRemoved = mp_impl->m_books.erase(id) == 1; + const bool bookWasRemoved = m_books.erase(id) == 1; if ( bookWasRemoved ) { - ++mp_impl->m_revision; + ++m_revision; } return bookWasRemoved; } Library::Revision Library::getRevision() const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - return mp_impl->m_revision; + std::lock_guard<std::mutex> lock(m_mutex); + return m_revision; } uint32_t Library::removeBooksNotUpdatedSince(Revision libraryRevision) { BookIdCollection booksToRemove; { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - for ( const auto& entry : mp_impl->m_books) { + std::lock_guard<std::mutex> lock(m_mutex); + for ( const auto& entry : m_books) { if ( entry.second.lastUpdatedRevision <= libraryRevision ) { booksToRemove.push_back(entry.first); } @@ -256,12 +212,12 @@ const Book& Library::getBookById(const std::string& id) const { // XXX: Doesn't make sense to lock this operation since it cannot // XXX: guarantee thread-safety because of its return type - return mp_impl->m_books.at(id); + return m_books.at(id); } Book Library::getBookByIdThreadSafe(const std::string& id) const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); return getBookById(id); } @@ -269,7 +225,7 @@ const Book& Library::getBookByPath(const std::string& path) const { // XXX: Doesn't make sense to lock this operation since it cannot // XXX: guarantee thread-safety because of its return type - for(auto& it: mp_impl->m_books) { + for(auto& it: m_books) { auto& book = it.second; if (book.getPath() == path) return book; @@ -282,7 +238,7 @@ const Book& Library::getBookByPath(const std::string& path) const std::shared_ptr<zim::Archive> Library::getArchiveById(const std::string& id) { try { - return mp_impl->mp_archiveCache->getOrPut(id, + return mp_archiveCache->getOrPut(id, [&](){ auto book = getBookById(id); if (!book.isPathValid()) { @@ -299,7 +255,7 @@ std::shared_ptr<ZimSearcher> Library::getSearcherByIds(const BookIdSet& ids) { assert(!ids.empty()); try { - return mp_impl->mp_searcherCache->getOrPut(ids, + return mp_searcherCache->getOrPut(ids, [&](){ std::vector<zim::Archive> archives; for(auto& id:ids) { @@ -319,8 +275,8 @@ std::shared_ptr<ZimSearcher> Library::getSearcherByIds(const BookIdSet& ids) unsigned int Library::getBookCount(const bool localBooks, const bool remoteBooks) const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - return mp_impl->getBookCount(localBooks, remoteBooks); + std::lock_guard<std::mutex> lock(m_mutex); + return getBookCount_not_protected(localBooks, remoteBooks); } bool Library::writeToFile(const std::string& path) const @@ -332,7 +288,7 @@ bool Library::writeToFile(const std::string& path) const dumper.setBaseDir(baseDir); std::string xml; { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); xml = dumper.dumpLibXMLContent(allBookIds); }; return writeTextFile(path, xml); @@ -348,10 +304,10 @@ bool Library::writeBookmarksToFile(const std::string& path) const Library::AttributeCounts Library::getBookAttributeCounts(BookStrPropMemFn p) const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); AttributeCounts propValueCounts; - for (const auto& pair: mp_impl->m_books) { + for (const auto& pair: m_books) { const auto& book = pair.second; if (book.getOrigId().empty()) { propValueCounts[(book.*p)()] += 1; @@ -380,10 +336,10 @@ std::vector<std::string> Library::getBooksLanguages() const Library::AttributeCounts Library::getBooksLanguagesWithCounts() const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); AttributeCounts langsWithCounts; - for (const auto& pair: mp_impl->m_books) { + for (const auto& pair: m_books) { const auto& book = pair.second; if (book.getOrigId().empty()) { for ( const auto& lang : book.getLanguages() ) { @@ -396,10 +352,10 @@ Library::AttributeCounts Library::getBooksLanguagesWithCounts() const std::vector<std::string> Library::getBooksCategories() const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); std::set<std::string> categories; - for (const auto& pair: mp_impl->m_books) { + for (const auto& pair: m_books) { const auto& book = pair.second; const auto& c = book.getCategory(); if ( !c.empty() ) { @@ -423,12 +379,12 @@ std::vector<std::string> Library::getBooksPublishers() const const std::vector<kiwix::Bookmark> Library::getBookmarks(bool onlyValidBookmarks) const { if (!onlyValidBookmarks) { - return mp_impl->m_bookmarks; + return m_bookmarks; } std::vector<kiwix::Bookmark> validBookmarks; auto booksId = getBooksIds(); - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - for(auto& bookmark:mp_impl->m_bookmarks) { + std::lock_guard<std::mutex> lock(m_mutex); + for(auto& bookmark:m_bookmarks) { if (std::find(booksId.begin(), booksId.end(), bookmark.getBookId()) != booksId.end()) { validBookmarks.push_back(bookmark); } @@ -438,10 +394,10 @@ const std::vector<kiwix::Bookmark> Library::getBookmarks(bool onlyValidBookmarks Library::BookIdCollection Library::getBooksIds() const { - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); BookIdCollection bookIds; - for (auto& pair: mp_impl->m_books) { + for (auto& pair: m_books) { bookIds.push_back(pair.first); } @@ -496,7 +452,7 @@ void Library::updateBookDB(const Book& book) doc.set_data(book.getId()); - mp_impl->m_bookDB.replace_document(idterm, doc); + m_bookDB->replace_document(idterm, doc); } namespace @@ -644,10 +600,10 @@ Library::BookIdCollection Library::filterViaBookDB(const Filter& filter) const BookIdCollection bookIds; - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); - Xapian::Enquire enquire(mp_impl->m_bookDB); + std::lock_guard<std::mutex> lock(m_mutex); + Xapian::Enquire enquire(*m_bookDB); enquire.set_query(query); - const auto results = enquire.get_mset(0, mp_impl->m_books.size()); + const auto results = enquire.get_mset(0, m_books.size()); for ( auto it = results.begin(); it != results.end(); ++it ) { bookIds.push_back(it.get_document().get_data()); } @@ -659,9 +615,9 @@ Library::BookIdCollection Library::filter(const Filter& filter) const { BookIdCollection result; const auto preliminaryResult = filterViaBookDB(filter); - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); for(auto id : preliminaryResult) { - if(filter.accept(mp_impl->m_books.at(id))) { + if(filter.accept(m_books.at(id))) { result.push_back(id); } } @@ -733,7 +689,7 @@ void Library::sort(BookIdCollection& bookIds, supportedListSortBy sort, bool asc // NOTE: for the entire duration of the sort. Will need to obtain (under a // NOTE: lock) the required atributes from the books once, and then the // NOTE: sorting will run on a copy of data without locking. - std::lock_guard<std::mutex> lock(mp_impl->m_mutex); + std::lock_guard<std::mutex> lock(m_mutex); switch(sort) { case TITLE: std::sort(bookIds.begin(), bookIds.end(), Comparator<TITLE>(this, ascending)); From 1dc97055974a95245e17d14cf27e6376cb8ca416 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier <mgautier@kymeria.fr> Date: Wed, 23 Aug 2023 11:03:16 +0200 Subject: [PATCH 10/11] Introduce LibraryPtr and ConstLibraryPtr. As we enforce the use of Library through a shared_ptr, let's simplify user life (and code) with new "type". --- include/library.h | 8 ++++++-- include/manager.h | 8 ++++---- src/manager.cpp | 4 ++-- src/name_mapper.cpp | 2 +- src/server.cpp | 2 +- src/server/internalServer.cpp | 2 +- src/server/internalServer.h | 4 ++-- 7 files changed, 17 insertions(+), 13 deletions(-) diff --git a/include/library.h b/include/library.h index 795792b33..f1a58f6c5 100644 --- a/include/library.h +++ b/include/library.h @@ -183,6 +183,9 @@ class ConcurrentCache; template<typename, typename> class MultiKeyCache; +using LibraryPtr = std::shared_ptr<Library>; +using ConstLibraryPtr = std::shared_ptr<const Library>; + /** * A Library store several books. */ @@ -198,8 +201,8 @@ class Library: public std::enable_shared_from_this<Library> Library(); public: - [[nodiscard]] static std::shared_ptr<Library> create() { - return std::shared_ptr<Library>(new Library()); + [[nodiscard]] static LibraryPtr create() { + return LibraryPtr(new Library()); } ~Library(); @@ -405,6 +408,7 @@ private: //data std::unique_ptr<Xapian::WritableDatabase> m_bookDB; }; + } #endif diff --git a/include/manager.h b/include/manager.h index 9b13da39c..415db6224 100644 --- a/include/manager.h +++ b/include/manager.h @@ -37,10 +37,10 @@ namespace kiwix class LibraryManipulator { public: // functions - explicit LibraryManipulator(std::shared_ptr<Library> library); + explicit LibraryManipulator(LibraryPtr library); virtual ~LibraryManipulator(); - std::shared_ptr<Library> getLibrary() const { return library; } + LibraryPtr getLibrary() const { return library; } bool addBookToLibrary(const Book& book); void addBookmarkToLibrary(const Bookmark& bookmark); @@ -52,7 +52,7 @@ class LibraryManipulator virtual void booksWereRemovedFromLibrary(); private: // data - std::shared_ptr<kiwix::Library> library; + LibraryPtr library; }; /** @@ -65,7 +65,7 @@ class Manager public: // functions explicit Manager(LibraryManipulator manipulator); - explicit Manager(std::shared_ptr<Library> library); + explicit Manager(LibraryPtr library); /** * Read a `library.xml` and add book in the file to the library. diff --git a/src/manager.cpp b/src/manager.cpp index 9a4f9b28c..ba4d0c75f 100644 --- a/src/manager.cpp +++ b/src/manager.cpp @@ -31,7 +31,7 @@ namespace kiwix // LibraryManipulator //////////////////////////////////////////////////////////////////////////////// -LibraryManipulator::LibraryManipulator(std::shared_ptr<Library> library) +LibraryManipulator::LibraryManipulator(LibraryPtr library) : library(library) {} @@ -85,7 +85,7 @@ Manager::Manager(LibraryManipulator manipulator): { } -Manager::Manager(std::shared_ptr<Library> library) : +Manager::Manager(LibraryPtr library) : writableLibraryPath(""), manipulator(LibraryManipulator(library)) { diff --git a/src/name_mapper.cpp b/src/name_mapper.cpp index f44227042..bf2a2e4dd 100644 --- a/src/name_mapper.cpp +++ b/src/name_mapper.cpp @@ -63,7 +63,7 @@ std::string HumanReadableNameMapper::getIdForName(const std::string& name) const // UpdatableNameMapper //////////////////////////////////////////////////////////////////////////////// -UpdatableNameMapper::UpdatableNameMapper(std::shared_ptr<Library> lib, bool withAlias) +UpdatableNameMapper::UpdatableNameMapper(LibraryPtr lib, bool withAlias) : library(lib) , withAlias(withAlias) { diff --git a/src/server.cpp b/src/server.cpp index 2e258245f..1a4ecfa9e 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -29,7 +29,7 @@ namespace kiwix { -Server::Server(std::shared_ptr<Library> library, std::shared_ptr<NameMapper> nameMapper) : +Server::Server(LibraryPtr library, std::shared_ptr<NameMapper> nameMapper) : mp_library(library), mp_nameMapper(nameMapper), mp_server(nullptr) diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 11bdd0971..08d35e702 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -411,7 +411,7 @@ public: }; -InternalServer::InternalServer(std::shared_ptr<Library> library, +InternalServer::InternalServer(LibraryPtr library, std::shared_ptr<NameMapper> nameMapper, std::string addr, int port, diff --git a/src/server/internalServer.h b/src/server/internalServer.h index e11c690b5..cdd7cebe2 100644 --- a/src/server/internalServer.h +++ b/src/server/internalServer.h @@ -92,7 +92,7 @@ class OPDSDumper; class InternalServer { public: - InternalServer(std::shared_ptr<Library> library, + InternalServer(LibraryPtr library, std::shared_ptr<NameMapper> nameMapper, std::string addr, int port, @@ -178,7 +178,7 @@ class InternalServer { int m_ipConnectionLimit; struct MHD_Daemon* mp_daemon; - std::shared_ptr<Library> mp_library; + LibraryPtr mp_library; std::shared_ptr<NameMapper> mp_nameMapper; SearchCache searchCache; From 9166b67c47f5e602063858835799788610413e80 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier <mgautier@kymeria.fr> Date: Tue, 19 Sep 2023 16:45:11 +0200 Subject: [PATCH 11/11] Do not allow SearchRendered to work on a delete nameMapper/Library. By moving the nameMapper/library arguments in `getHtml`/`getXml` we avoid any potential "use after free" of name mapper or library as they are not stored. --- include/search_renderer.h | 42 +++++++++++++---------------------- src/search_renderer.cpp | 25 ++++++++------------- src/server/internalServer.cpp | 14 +++++++++--- 3 files changed, 36 insertions(+), 45 deletions(-) diff --git a/include/search_renderer.h b/include/search_renderer.h index d26d571a9..2669b84cf 100644 --- a/include/search_renderer.h +++ b/include/search_renderer.h @@ -37,29 +37,11 @@ class SearchRenderer /** * Construct a SearchRenderer from a SearchResultSet. * - * The constructed version of the SearchRenderer will not introduce - * the book name for each result. It is better to use the other constructor - * with a Library pointer to have a better html page. - * * @param srs The `SearchResultSet` to render. - * @param mapper The `NameMapper` to use to do the rendering. * @param start The start offset used for the srs. * @param estimatedResultCount The estimatedResultCount of the whole search */ - SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, - unsigned int start, unsigned int estimatedResultCount); - - /** - * Construct a SearchRenderer from a SearchResultSet. - * - * @param srs The `SearchResultSet` to render. - * @param mapper The `NameMapper` to use to do the rendering. - * @param library The `Library` to use to look up book details for search results. - * @param start The start offset used for the srs. - * @param estimatedResultCount The estimatedResultCount of the whole search - */ - SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, Library* library, - unsigned int start, unsigned int estimatedResultCount); + SearchRenderer(zim::SearchResultSet srs, unsigned int start, unsigned int estimatedResultCount); ~SearchRenderer(); @@ -90,24 +72,32 @@ class SearchRenderer this->pageLength = pageLength; } - std::string renderTemplate(const std::string& tmpl_str); - /** * Generate the html page with the resutls of the search. + * + * @param mapper The `NameMapper` to use to do the rendering. + * @param library The `Library` to use to look up book details for search results. + May be nullptr. In this case, bookName is not set in the rendered string. + * @return The html string */ - std::string getHtml(); + std::string getHtml(const NameMapper& mapper, const Library* library); - /** + /** * Generate the xml page with the resutls of the search. + * + * @param mapper The `NameMapper` to use to do the rendering. + * @param library The `Library` to use to look up book details for search results. + May be nullptr. In this case, bookName is not set in the rendered string. + * @return The xml string */ - std::string getXml(); + std::string getXml(const NameMapper& mapper, const Library* library); + protected: // function + std::string renderTemplate(const std::string& tmpl_str, const NameMapper& mapper, const Library *library); protected: std::string beautifyInteger(const unsigned int number); zim::SearchResultSet m_srs; - const NameMapper* mp_nameMapper; - Library* mp_library; std::string searchBookQuery; std::string searchPattern; std::string protocolPrefix; diff --git a/src/search_renderer.cpp b/src/search_renderer.cpp index ab49a8c8c..3b2ae161b 100644 --- a/src/search_renderer.cpp +++ b/src/search_renderer.cpp @@ -36,16 +36,9 @@ namespace kiwix { /* Constructor */ -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, - unsigned int start, unsigned int estimatedResultCount) - : SearchRenderer(srs, mapper, nullptr, start, estimatedResultCount) -{} - -SearchRenderer::SearchRenderer(zim::SearchResultSet srs, const NameMapper* mapper, Library* library, +SearchRenderer::SearchRenderer(zim::SearchResultSet srs, unsigned int start, unsigned int estimatedResultCount) : m_srs(srs), - mp_nameMapper(mapper), - mp_library(library), protocolPrefix("zim://"), searchProtocolPrefix("search://"), estimatedResultCount(estimatedResultCount), @@ -164,7 +157,7 @@ kainjow::mustache::data buildPagination( return pagination; } -std::string SearchRenderer::renderTemplate(const std::string& tmpl_str) +std::string SearchRenderer::renderTemplate(const std::string& tmpl_str, const NameMapper& nameMapper, const Library* library) { const std::string absPathPrefix = protocolPrefix; // Build the results list @@ -172,12 +165,12 @@ std::string SearchRenderer::renderTemplate(const std::string& tmpl_str) for (auto it = m_srs.begin(); it != m_srs.end(); it++) { kainjow::mustache::data result; const std::string zim_id(it.getZimId()); - const auto path = mp_nameMapper->getNameForId(zim_id) + "/" + it.getPath(); + const auto path = nameMapper.getNameForId(zim_id) + "/" + it.getPath(); result.set("title", it.getTitle()); result.set("absolutePath", absPathPrefix + urlEncode(path)); result.set("snippet", it.getSnippet()); - if (mp_library) { - result.set("bookTitle", mp_library->getBookById(zim_id).getTitle()); + if (library) { + result.set("bookTitle", library->getBookById(zim_id).getTitle()); } if (it.getWordCount() >= 0) { result.set("wordCount", kiwix::beautifyInteger(it.getWordCount())); @@ -222,14 +215,14 @@ std::string SearchRenderer::renderTemplate(const std::string& tmpl_str) return ss.str(); } -std::string SearchRenderer::getHtml() +std::string SearchRenderer::getHtml(const NameMapper& mapper, const Library* library) { - return renderTemplate(RESOURCE::templates::search_result_html); + return renderTemplate(RESOURCE::templates::search_result_html, mapper, library); } -std::string SearchRenderer::getXml() +std::string SearchRenderer::getXml(const NameMapper& mapper, const Library* library) { - return renderTemplate(RESOURCE::templates::search_result_xml); + return renderTemplate(RESOURCE::templates::search_result_xml, mapper, library); } diff --git a/src/server/internalServer.cpp b/src/server/internalServer.cpp index 08d35e702..c8446cb17 100644 --- a/src/server/internalServer.cpp +++ b/src/server/internalServer.cpp @@ -963,7 +963,7 @@ std::unique_ptr<Response> InternalServer::handle_search_request(const RequestCon const auto pageLength = getSearchPageSize(request); /* Get the results */ - SearchRenderer renderer(search->getResults(start-1, pageLength), mp_nameMapper.get(), mp_library.get(), start, + SearchRenderer renderer(search->getResults(start-1, pageLength), start, search->getEstimatedMatches()); renderer.setSearchPattern(searchInfo.pattern); renderer.setSearchBookQuery(searchInfo.bookFilterQuery); @@ -971,9 +971,17 @@ std::unique_ptr<Response> InternalServer::handle_search_request(const RequestCon renderer.setSearchProtocolPrefix(m_root + "/search"); renderer.setPageLength(pageLength); if (request.get_requested_format() == "xml") { - return ContentResponse::build(*this, renderer.getXml(), "application/rss+xml; charset=utf-8"); + return ContentResponse::build( + *this, + renderer.getXml(*mp_nameMapper, mp_library.get()), + "application/rss+xml; charset=utf-8" + ); } - auto response = ContentResponse::build(*this, renderer.getHtml(), "text/html; charset=utf-8"); + auto response = ContentResponse::build( + *this, + renderer.getHtml(*mp_nameMapper, mp_library.get()), + "text/html; charset=utf-8" + ); // XXX: Now this has to be handled by the iframe-based viewer which // XXX: has to resolve if the book selection resulted in a single book. /*