diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c66f75f..4935cdc9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add keyword-only `include_snippets=True` to `Search.get_results()`; passing `False` skips native snippet generation and leaves `SearchResult.snippet` empty (#298) + +### Changed + +- Release the GIL around archive construction and native full-text search operations, allowing independent reader workers to search concurrently (#298) + ## [4.1.0] - 2026-11-05 ### Added diff --git a/README.md b/README.md index 468f6052..56e1d7a3 100644 --- a/README.md +++ b/README.md @@ -159,9 +159,21 @@ with Creator("test.zim").config_indexing(True, "eng") as creator: #### Thread safety -> The reading part of the libzim is most of the time thread safe. Searching and creating part are not. [libzim documentation](https://libzim.readthedocs.io/en/latest/usage.html#introduction) - -`python-libzim` disables the [GIL](https://wiki.python.org/moin/GlobalInterpreterLock) on most of C++ libzim calls. You **must prevent concurrent access** yourself. This is easily done by wrapping all creator calls with a [`threading.Lock()`](https://docs.python.org/3/library/threading.html#lock-objects) +Archive construction, full-text result retrieval and match estimation, and native +full-text result iteration release the [GIL](https://wiki.python.org/moin/GlobalInterpreterLock). +Other reader accessors and suggestions do not generally release it. + +For parallel full-text searches, use a native libzim build with the +`InternalDataBase::lock()` mutex-acquisition fix, and give each worker its own +`Archive` opened from the filename, `Searcher`, query, and result iterator. +Separate searchers built from the same archive can share native database state +and serialize on its locks. Do not concurrently mutate a shared query or +searcher, or advance the same iterator. GIL release is not a blanket +thread-safety guarantee. + +The writer also releases the GIL around its blocking calls. You **must prevent +concurrent creator access** yourself, for example with a +[`threading.Lock()`](https://docs.python.org/3/library/threading.html#lock-objects): ```py lock = threading.Lock() diff --git a/libzim/libzim.pyx b/libzim/libzim.pyx index 5f60b50b..b7aa8148 100644 --- a/libzim/libzim.pyx +++ b/libzim/libzim.pyx @@ -1171,7 +1171,9 @@ cdef class Archive: filename : Full path to a zim file """ - self.c_archive = move(zim.Archive(str(filename).encode('UTF-8'))) + cdef string _filename = str(filename).encode('UTF-8') + with nogil: + self.c_archive = move(zim.Archive(_filename)) self._filename = pathlib.Path(self.c_archive.getFilename().decode("UTF-8", "strict")) def __eq__(self, other) -> pybool: @@ -1839,29 +1841,39 @@ cdef class SearchResultSet: __module__ = search_module_name cdef zim.SearchResultSet c_resultset cdef list _archives + cdef bool _include_snippets @staticmethod - cdef from_resultset(zim.SearchResultSet _resultset, list archives): + cdef from_resultset(zim.SearchResultSet _resultset, list archives, bool include_snippets): cdef SearchResultSet resultset = SearchResultSet() resultset.c_resultset = move(_resultset) resultset._archives = archives + resultset._include_snippets = include_snippets return resultset def __iter__(self) -> Iterator[SearchResult]: """Search results found in Archive(s) for Search""" - cdef zim.SearchIterator current = self.c_resultset.begin() - cdef zim.SearchIterator end = self.c_resultset.end() + cdef zim.SearchIterator current, end cdef int archive_idx cdef Archive archive - cdef str path, title, snippet + cdef string path, title, snippet + with nogil: + current = self.c_resultset.begin() + end = self.c_resultset.end() while current != end: - archive_idx = current.getFileIndex() + with nogil: + archive_idx = current.getFileIndex() + path = current.getPath() + title = current.getTitle() + if self._include_snippets: + snippet = current.getSnippet() archive = self._archives[archive_idx] - path = current.getPath().decode('UTF-8') - title = current.getTitle().decode('UTF-8') - snippet = current.getSnippet().decode('UTF-8') - yield SearchResult(archive, path, title, snippet) - preincrement(current) + yield SearchResult( + archive, path.decode('UTF-8'), title.decode('UTF-8'), + snippet.decode('UTF-8') + ) + with nogil: + preincrement(current) cdef class Search: @@ -1883,22 +1895,30 @@ cdef class Search: Returns: The number of estimated results for this search. """ - return self.c_search.getEstimatedMatches() + cdef int matches + with nogil: + matches = self.c_search.getEstimatedMatches() + return matches - def get_results(self, start: pyint, count: pyint) -> SearchResultSet: + def get_results( + self, start: pyint, count: pyint, *, bool include_snippets=True + ) -> SearchResultSet: """Iterator over search results found in Archive(s) for the search. Args: start (int): The beginning of the range to get (offset of the first result). count (int): The number of results to return. + include_snippets (bool): Generate result snippets (default: True). Returns: A set of results for this search. """ - return SearchResultSet.from_resultset( - move(self.c_search.getResults(start, count)), - self._archives - ) + cdef int _start = start + cdef int _count = count + cdef zim.SearchResultSet results + with nogil: + results = move(self.c_search.getResults(_start, _count)) + return SearchResultSet.from_resultset(move(results), self._archives, include_snippets) cdef class Searcher: @@ -1951,7 +1971,7 @@ cdef class Searcher: """ return Search.from_search( move(self.c_searcher.search((query).c_query)), - self._archives + self._archives.copy() ) diff --git a/libzim/search.pyi b/libzim/search.pyi index d97f40df..2ba07eed 100644 --- a/libzim/search.pyi +++ b/libzim/search.pyi @@ -7,7 +7,13 @@ from typing_extensions import disjoint_base from libzim.reader import Archive -__all__ = ["Query", "Search", "SearchResult", "SearchResultSet", "Searcher"] +__all__ = [ + "Query", + "Search", + "SearchResult", + "SearchResultSet", + "Searcher", +] @disjoint_base class Query: @@ -30,7 +36,9 @@ class SearchResultSet: @disjoint_base class Search: def get_estimated_matches(self) -> int: ... - def get_results(self, start: int, count: int) -> SearchResultSet: ... + def get_results( + self, start: int, count: int, *, include_snippets: bool = True + ) -> SearchResultSet: ... @disjoint_base class Searcher: diff --git a/libzim/zim.pxd b/libzim/zim.pxd index 73a6420d..f13a34f3 100644 --- a/libzim/zim.pxd +++ b/libzim/zim.pxd @@ -101,13 +101,13 @@ cdef extern from "zim/search.h" namespace "zim": cdef extern from "zim/search_iterator.h" namespace "zim": cdef cppclass SearchIterator: SearchIterator() - SearchIterator operator++() - bint operator==(SearchIterator) - bint operator!=(SearchIterator) - string getPath() - string getTitle() - string getSnippet() - int getFileIndex() + SearchIterator operator++() except + nogil + bint operator==(SearchIterator) except + nogil + bint operator!=(SearchIterator) except + nogil + string getPath() except + nogil + string getTitle() except + nogil + string getSnippet() except + nogil + int getFileIndex() except + nogil # Import the python wrappers (ObjWrapper) from libwrapper. @@ -199,7 +199,7 @@ cdef extern from "libwrapper.h" namespace "wrapper": ctypedef vector[IllustrationInfo] IllustrationInfos Archive() except + - Archive(string filename) except + + Archive(string filename) except + nogil uint64_t getFilesize() except + @@ -258,13 +258,13 @@ cdef extern from "libwrapper.h" namespace "wrapper": Search search(Query query) except + cdef cppclass Search: - int getEstimatedMatches() except + - SearchResultSet getResults(int start, int count) except + + int getEstimatedMatches() except + nogil + SearchResultSet getResults(int start, int count) except + nogil cdef cppclass SearchResultSet: - SearchIterator begin() - SearchIterator end() - int size() + SearchIterator begin() except + nogil + SearchIterator end() except + nogil + int size() except + nogil cdef cppclass SuggestionItem: string getPath() diff --git a/tests/test_libzim_creator.py b/tests/test_libzim_creator.py index 55ea44e9..dbb4008d 100644 --- a/tests/test_libzim_creator.py +++ b/tests/test_libzim_creator.py @@ -26,6 +26,8 @@ import platform import subprocess import sys +import threading +from concurrent.futures import ThreadPoolExecutor import libzim from libzim.illustration import ( # pyright: ignore [reportMissingModuleSource] @@ -1117,6 +1119,11 @@ def test_multi_archive_search_and_snippets(tmpdir): assert search.get_estimated_matches() == 2 results = list(search.get_results(0, 10)) assert len(results) == 2 + without_snippets = list(search.get_results(0, 10, include_snippets=False)) + assert [(r.archive, r.path, r.title) for r in without_snippets] == [ + (r.archive, r.path, r.title) for r in results + ] + assert all(r.snippet == "" for r in without_snippets) for r in results: assert isinstance(r, SearchResult) @@ -1157,3 +1164,88 @@ def test_multi_archive_search_and_snippets(tmpdir): multi_searcher_tuple = Searcher((zim1, zim2)) assert multi_searcher_tuple.search(query).get_estimated_matches() == 2 + + +@pytest.mark.parametrize("include_snippets", [False, True]) +def test_concurrent_search_with_independent_handles(fpath, include_snippets): + with Creator(fpath).config_indexing(True, "eng") as creator: + for index in range(8): + creator.add_item( + StaticItem( + path=f"article{index}", + title=f"Article {index}", + content=f"

Searchable content for article {index}

", + mimetype="text/html", + ) + ) + + ready = threading.Barrier(4) + + def search_worker(_): + archive = Archive(fpath) + searcher = Searcher(archive) + ready.wait(timeout=10) + batches = [] + for _ in range(5): + search = searcher.search(Query().set_query("searchable")) + assert search.get_estimated_matches() == 8 + batches.append( + [ + (r.path, r.title, r.snippet) + for r in search.get_results(0, 8, include_snippets=include_snippets) + ] + ) + return batches + + with ThreadPoolExecutor(4) as executor: + batches = list(executor.map(search_worker, range(4))) + assert all(batch == batches[0][0] for worker in batches for batch in worker) + assert len(batches[0][0]) == 8 + + +def test_search_iteration_releases_gil(fpath): + if hasattr(sys, "_is_gil_enabled") and not sys._is_gil_enabled(): + pytest.skip("requires a GIL-enabled interpreter") + with Creator(fpath).config_indexing(True, "eng") as creator: + creator.add_item( + StaticItem( + path="article", + title="Article", + content="

Searchable content.

" * 100000, + mimetype="text/html", + ) + ) + + archive = Archive(fpath) + search = Searcher(archive).search(Query().set_query("searchable")) + results = search.get_results(0, 1) + stop = threading.Event() + ready = threading.Event() + ticks = [] + + def observer(): + ready.set() + while not stop.wait(0.0001): + ticks.append(None) + + thread = threading.Thread(target=observer) + thread.start() + ready.wait(timeout=5) + interval = sys.getswitchinterval() + try: + sys.setswitchinterval(60) + before = len(ticks) + # A single snippet may be too short for the observer to be scheduled + # after the GIL is released. Retry a few times so the test is not + # dependent on one scheduling window; if the GIL is never released, + # ticks cannot advance at all under the raised switch interval. + for _ in range(10): + assert next(iter(results)).snippet + if len(ticks) > before: + break + progressed = len(ticks) > before + finally: + sys.setswitchinterval(interval) + stop.set() + thread.join(timeout=5) + assert progressed, "GIL was not released while generating snippets"