Support for library reloading #636

veloman-yunkan · 2021-11-20T17:31:28Z

Enables kiwix/kiwix-tools#497 to fix kiwix/kiwix-tools#243

This PR provides facilities for reloading and updating the library with any of

new books
removed books
any changes in book metadata.

The PR also includes various improvements/bugfixes noticed while working on this PR, as well as a few new unit-tests.

API changes introduced by this PR are addressed in kiwix-desktop by kiwix/kiwix-desktop#732.

Reused `Library::getArchiveById()` in `Library::getReaderById()`.

codecov · 2021-11-20T17:32:19Z

Codecov Report

Merging #636 (3a127e4) into master (c7b8839) will increase coverage by 0.55%.
The diff coverage is 76.64%.

❗ Current head 3a127e4 differs from pull request most recent head 405ea29. Consider uploading reports for the commit 405ea29 to get more accurate results

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
+ Coverage   66.62%   67.17%   +0.55%     
==========================================
  Files          53       53              
  Lines        3823     3906      +83     
  Branches     1931     1979      +48     
==========================================
+ Hits         2547     2624      +77     
- Misses       1274     1281       +7     
+ Partials        2        1       -1

Impacted Files	Coverage Δ
include/name_mapper.h	`71.42% <33.33%> (+4.76%)`	⬆️
include/library.h	`92.30% <50.00%> (+0.64%)`	⬆️
src/manager.cpp	`77.27% <72.72%> (-0.71%)`	⬇️
src/library.cpp	`77.72% <73.33%> (-0.79%)`	⬇️
include/manager.h	`100.00% <100.00%> (+22.22%)`	⬆️
src/book.cpp	`93.10% <100.00%> (ø)`
src/name_mapper.cpp	`100.00% <100.00%> (+50.00%)`	⬆️
src/opds_dumper.cpp	`100.00% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7b8839...405ea29. Read the comment docs.

New unit test BookTest.getHumanReadableIdFromPath revealed a bug in `Book::getHumanReadableIdFromPath()`.

mgautierfr

I've nothing to say about the quality of the code itself here. Really good as usual.

I'm a bit disturbed by the ReloadableLibrary.
The idea was to have the Library as a "simple" container of book and have all manipulation of the library put elsewhere (Manager/Manipulator)
The ReloadableLibray takes the opposite direction by creating a library which knows from what is was created and can reload itself.
I would have extend the Manager to be able to do a "diff" between the "library.xml" it is reading and the Library it is managing and apply this "diff" to the Library

What about multithreading ?
We will reload the library while a server will run in the same time.
We must be prepared to concurrent access/modifications.

And we need book removal.
The new library.xml will have new books added and old books removed. We must correctly handle this

include/name_mapper.h

src/library.cpp

include/reloadable_library.h

Fixed the build failure caused by the libkiwix API changes done in kiwix/libkiwix#636.

veloman-yunkan · 2021-11-28T17:03:49Z

@mgautierfr

What about multithreading ? We will reload the library while a server will run in the same time. We must be prepared to concurrent access/modifications.

Library is now thread safe as long as its usage in kiwix-serve is concerned.

And we need book removal. The new library.xml will have new books added and old books removed. We must correctly handle this

Added prototype support for book removal during library reloading that neglects LibraryManipulator. If you don't object against my approach, then respecting LibraryManipulator should not be a problem, though there are several options to do that.

mgautierfr

If you don't object against my approach, then respecting LibraryManipulator should not be a problem, though there are several options to do that.

Marking the book updated is a good enough way to detect books not updated.

In this PR you modify a bit the LibraryManipulator to provide a default implementation and also a bit the behavior of the Manager (The Manager is not a one shoot object but it is keep alive to be reused).
I'm not against those changes, on the contrary. If you think we can go further to reorganize all those different classes, please do not hesitate to do it (maybe not in this PR, or open a discussion to be sure about what we can do).

src/library.cpp

mgautierfr · 2021-11-29T10:27:51Z

src/library.cpp

+    if ( ! booksReferToTheSameArchive(oldbook, book) ) {
+      dropReader(book.getId());
+    }
+    oldbook.update(book); // XXX: This may have no effect if oldbook is readonly


I don't know why we have this "readonly" attribute on the book.
This is something pretty old and we don't use it at all. Maybe @kelson42 knows why it has been introduced.

But if you want to drop it, I agree 🙂

If @kelson42 doesn't come up with arguments against removing Book::m_readOnly I will do it in a separate PR.

src/library.cpp

mgautierfr · 2021-11-30T13:38:52Z

Please rebase-fixup and we can merge.

The right place for NameMapperProxy introduced by kiwix/kiwix-desktop#714 is in libkiwix (so that it can be reused in kiwix-serve).

Originally `LibraryManipulator` was an abstract class completely decoupled from `Library`. Its `addBookToLibrary()` and `addBookmarkToLibrary()` methods could be defined in an arbitrary way. Now `LibraryManipulator` has to be bound to a library object, those methods are no longer virtual, they always update the library and allow for some additional actions via virtual functions `bookWasAddedToLibrary()` and `bookmarkWasAddedToLibrary()`.

Introducing a mutex in `Library` necessitates manually implementing the move constructor and assignment operator. It's better to still delegate that work to the compiler to eliminate any possibility of bugs when new data members are added to `Library`. The trick is to move the data into an auxiliary class `LibraryBase` and derive `Library` from it.

Library became thread-safe with the exception of `getBookById()` and `getBookByPath()` methods - thread safety in those accessors is rendered meaningless by their return type (they return a reference to a book which can be removed any time later by another thread).

veloman-yunkan · 2021-11-30T14:30:04Z

Please rebase-fixup and we can merge.

Done (also fixed a few typos noticed during the final quick review).

Fixed the build failure caused by the libkiwix API changes done in kiwix/libkiwix#636.

veloman-yunkan added 3 commits November 19, 2021 20:17

Simplified Library::getReaderById()

0e48baf

Reused `Library::getArchiveById()` in `Library::getReaderById()`.

Made Manager's ctors explicit

913a368

Manager is now safe to copy

571e417

kelson42 mentioned this pull request Nov 21, 2021

Detect and add new zim files from Download Dir into local library kiwix/kiwix-desktop#712

Closed

veloman-yunkan mentioned this pull request Nov 21, 2021

Library reloading on SIGHUP kiwix/kiwix-tools#497

Merged

veloman-yunkan force-pushed the library_reloading branch from c2c35d3 to 6884386 Compare November 22, 2021 14:59

veloman-yunkan added 4 commits November 22, 2021 20:54

Testing of Book::getHumanReadableIdFromPath()

3296a02

New unit test BookTest.getHumanReadableIdFromPath revealed a bug in `Book::getHumanReadableIdFromPath()`.

Bugfix in Book::getHumanReadableIdFromPath()

339f845

Testing of HumanReadableNameMapper

d62c4fd

NameMapper's API is now const

5f3c34e

veloman-yunkan force-pushed the library_reloading branch from 6884386 to 191096c Compare November 22, 2021 17:12

veloman-yunkan changed the title ~~[WIP] Support for library reloading~~ Support for library reloading (without handling book removal). Nov 22, 2021

veloman-yunkan marked this pull request as ready for review November 22, 2021 17:23

veloman-yunkan requested a review from mgautierfr November 22, 2021 17:23

mgautierfr requested changes Nov 26, 2021

View reviewed changes

include/name_mapper.h Outdated Show resolved Hide resolved

src/library.cpp Show resolved Hide resolved

include/reloadable_library.h Outdated Show resolved Hide resolved

veloman-yunkan added a commit to kiwix/kiwix-desktop that referenced this pull request Nov 28, 2021

Addressed libkiwix API changes

61bdd86

Fixed the build failure caused by the libkiwix API changes done in kiwix/libkiwix#636.

veloman-yunkan changed the title ~~Support for library reloading (without handling book removal).~~ Support for library reloading Nov 28, 2021

veloman-yunkan requested a review from mgautierfr November 28, 2021 16:59

veloman-yunkan force-pushed the library_reloading branch from 8078190 to 900eb8e Compare November 28, 2021 17:07

veloman-yunkan mentioned this pull request Nov 28, 2021

Addressed libkiwix API changes kiwix/kiwix-desktop#732

Merged

veloman-yunkan force-pushed the library_reloading branch from 900eb8e to 95658fb Compare November 28, 2021 17:11

mgautierfr reviewed Nov 29, 2021

View reviewed changes

veloman-yunkan requested a review from mgautierfr November 29, 2021 17:39

veloman-yunkan added 4 commits November 30, 2021 18:17

Code deduplication in NameMapperTest

4ccbdcb

Added NameMapperProxy from kiwix/kiwix-desktop#714

8fffa59

The right place for NameMapperProxy introduced by kiwix/kiwix-desktop#714 is in libkiwix (so that it can be reused in kiwix-serve).

NameMapperProxy respects the withAlias flag

6199c11

Testing of NameMapperProxy

2d6a7fe

veloman-yunkan added 13 commits November 30, 2021 18:20

Library::addBook() updates the reader cache

76a5e3a

Manager::reload()

3aeeeee

Renamed NameMapperProxy -> UpdatableNameMapper

298247c

Dropped Library::getBookBy*() non-const functions

b712c73

Introduced Library::getBookByIdThreadSafe()

473d2d2

Thread safe dumping of the OPDS feed

ad2eb52

Noted a potential bug in Library::addBook()

1d53834

Enter Library::removeBooksNotUpdatedSince()

262e138

Manager::reload() also removes books from Library

7161db8

Added Library::addOrUpdateBook() alias

405ea29

veloman-yunkan force-pushed the library_reloading branch from 3a127e4 to 405ea29 Compare November 30, 2021 14:23

mgautierfr approved these changes Nov 30, 2021

View reviewed changes

mgautierfr merged commit 01ac0b2 into master Nov 30, 2021

mgautierfr deleted the library_reloading branch November 30, 2021 17:08

veloman-yunkan mentioned this pull request Nov 30, 2021

kiwix-serve should reload automatically content (library/files) kiwix/kiwix-tools#243

Closed

mgautierfr pushed a commit to kiwix/kiwix-desktop that referenced this pull request Dec 1, 2021

Addressed libkiwix API changes

9c467d7

Fixed the build failure caused by the libkiwix API changes done in kiwix/libkiwix#636.

kelson42 mentioned this pull request Dec 4, 2021

Monitor ZIM files from a user-chosen directory kiwix/kiwix-desktop#713

Closed

veloman-yunkan mentioned this pull request Dec 5, 2021

Deadlock in Library::writeBookmarksToFile() #641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for library reloading #636

Support for library reloading #636

veloman-yunkan commented Nov 20, 2021 •

edited

codecov bot commented Nov 20, 2021 •

edited

mgautierfr left a comment

veloman-yunkan commented Nov 28, 2021

mgautierfr left a comment

mgautierfr Nov 29, 2021

veloman-yunkan Nov 29, 2021

mgautierfr commented Nov 30, 2021

veloman-yunkan commented Nov 30, 2021

Support for library reloading #636

Support for library reloading #636

Conversation

veloman-yunkan commented Nov 20, 2021 • edited

codecov bot commented Nov 20, 2021 • edited

Codecov Report

mgautierfr left a comment

Choose a reason for hiding this comment

veloman-yunkan commented Nov 28, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Nov 29, 2021

Choose a reason for hiding this comment

veloman-yunkan Nov 29, 2021

Choose a reason for hiding this comment

mgautierfr commented Nov 30, 2021

veloman-yunkan commented Nov 30, 2021

veloman-yunkan commented Nov 20, 2021 •

edited

codecov bot commented Nov 20, 2021 •

edited