From 8ed926d78e28b9f6e773844837ee8c553a5ca08e Mon Sep 17 00:00:00 2001 From: renaud gaudin Date: Fri, 26 Jun 2020 17:29:41 +0000 Subject: [PATCH 1/2] Forward exceptions from Article's virtual methods Upon adding an article, virtual methods get_xxx() are called on the article to retrieve data. Should an exception be raised in this user code, it was caught and ignored. This changes this behavior to raise a RuntimeError with an explicit message instead. As this exception passes through Cython, we can't retrieve the actually raised exception though. --- libzim/lib.cxx | 16 ++++++++++++---- libzim/wrapper.pyx | 24 ++++++++++++++++++++---- tests/test_libzim.py | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/libzim/lib.cxx b/libzim/lib.cxx index 58af0a11..04825431 100644 --- a/libzim/lib.cxx +++ b/libzim/lib.cxx @@ -65,8 +65,10 @@ std::string ZimArticleWrapper::callCythonReturnString(std::string methodName) co int error; std::string ret_val = string_cy_call_fct(this->m_obj, methodName, &error); - if (error) + if (error && error == 1) throw std::runtime_error("The pure virtual function " + methodName + " must be override"); + if (error) + throw std::runtime_error("The virtual function " + methodName + " threw an exception"); return ret_val; } @@ -79,8 +81,10 @@ zim::Blob ZimArticleWrapper::callCythonReturnBlob(std::string methodName) const int error; zim::Blob ret_val = blob_cy_call_fct(this->m_obj, methodName, &error); - if (error) + if (error && error == 1) throw std::runtime_error("The pure virtual function " + methodName + " must be override"); + if (error) + throw std::runtime_error("The virtual function " + methodName + " threw an exception"); return ret_val; } @@ -93,8 +97,10 @@ bool ZimArticleWrapper::callCythonReturnBool(std::string methodName) const int error; bool ret_val = bool_cy_call_fct(this->m_obj, methodName, &error); - if (error) + if (error && error == 1) throw std::runtime_error("The pure virtual function " + methodName + " must be override"); + if (error) + throw std::runtime_error("The virtual function " + methodName + " threw an exception"); return ret_val; } @@ -107,8 +113,10 @@ uint64_t ZimArticleWrapper::callCythonReturnInt(std::string methodName) const int error; int ret_val = int_cy_call_fct(this->m_obj, methodName, &error); - if (error) + if (error && error == 1) throw std::runtime_error("The pure virtual function " + methodName + " must be override"); + if (error) + throw std::runtime_error("The virtual function " + methodName + " threw an exception"); return ret_val; } diff --git a/libzim/wrapper.pyx b/libzim/wrapper.pyx index 5f3a9b16..e04fcab3 100644 --- a/libzim/wrapper.pyx +++ b/libzim/wrapper.pyx @@ -119,7 +119,11 @@ cdef public api: string string_cy_call_fct(object obj, string method, int *error) with gil: """Lookup and execute a pure virtual method on ZimArticle returning a string""" func = get_article_method_from_object(obj, method, error) - ret_str = func() + try: + ret_str = func() + except Exception: + error[0] = 2 + raise return ret_str.encode('UTF-8') wrapper.Blob blob_cy_call_fct(object obj, string method, int *error) with gil: @@ -127,18 +131,30 @@ cdef public api: cdef WritingBlob blob func = get_article_method_from_object(obj, method, error) - blob = func() + try: + blob = func() + except Exception: + error[0] = 2 + raise return dereference(blob.c_blob) bool bool_cy_call_fct(object obj, string method, int *error) with gil: """Lookup and execute a pure virtual method on ZimArticle returning a bool""" func = get_article_method_from_object(obj, method, error) - return func() + try: + return func() + except Exception: + error[0] = 2 + raise uint64_t int_cy_call_fct(object obj, string method, int *error) with gil: """Lookup and execute a pure virtual method on ZimArticle returning an int""" func = get_article_method_from_object(obj, method, error) - return func() + try: + return func() + except Exception: + error[0] = 2 + raise cdef class Creator: """ Zim Creator diff --git a/tests/test_libzim.py b/tests/test_libzim.py index 9552baa8..d2c48a06 100644 --- a/tests/test_libzim.py +++ b/tests/test_libzim.py @@ -188,3 +188,36 @@ def test_filename_param_types(tmpdir): with Creator(str(path), "welcome") as creator: assert creator.filename == path assert isinstance(creator.filename, pathlib.Path) + + +def test_in_article_exceptions(tmpdir): + """ make sure we raise RuntimeError from article's virtual methods """ + + class BoolErrorArticle(SimpleArticle): + def is_redirect(self): + raise IOError + + class StringErrorArticle(SimpleArticle): + def get_url(self): + raise IOError + + class BlobErrorArticle(SimpleArticle): + def get_data(self): + raise IOError + + path, main_page = tmpdir / "test.zim", "welcome" + args = {"title": "Hello", "mime_type": "text/html", "content": "", "url": "welcome"} + + with Creator(path, main_page) as zim_creator: + # make sure we can can exception of all types (except int, not used) + with pytest.raises(RuntimeError): + zim_creator.add_article(BoolErrorArticle(**args)) + with pytest.raises(RuntimeError): + zim_creator.add_article(StringErrorArticle(**args)) + with pytest.raises(RuntimeError): + zim_creator.add_article(BlobErrorArticle(**args)) + + # make sure we can catch it from outside creator + with pytest.raises(RuntimeError): + with Creator(path, main_page) as zim_creator: + zim_creator.add_article(BlobErrorArticle(**args)) From a7e5232562b77c774ce97c4b4d10c81bd6760585 Mon Sep 17 00:00:00 2001 From: renaud gaudin Date: Fri, 26 Jun 2020 17:47:30 +0000 Subject: [PATCH 2/2] Test to ensure we can avoid ZIM file creation As the libzim lacks a `cancel()` method to properly stop a ZIM file creation process without calling `finalize()` (which always creates the ZIM file), users need to cheat the creator to not call finalize(), even upon exit (as `Creator`'s destructor do call `finalize()`) Test instanciates a different python VM as the libzim_reader tests (manual garbage collection) leads to a segfault. --- tests/test_libzim.py | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/tests/test_libzim.py b/tests/test_libzim.py index d2c48a06..9837148d 100644 --- a/tests/test_libzim.py +++ b/tests/test_libzim.py @@ -16,9 +16,12 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -import pytest +import sys +import subprocess import pathlib +import pytest + from libzim.writer import Article, Blob, Creator from libzim.reader import File @@ -221,3 +224,30 @@ def get_data(self): with pytest.raises(RuntimeError): with Creator(path, main_page) as zim_creator: zim_creator.add_article(BlobErrorArticle(**args)) + + +def test_dontcreatezim_onexception(tmpdir): + """ make sure we can prevent ZIM file creation (workaround missing cancel()) + + A new interpreter is instanciated to get a different memory space. + This workaround is not safe and may segfault at GC under some circumstances + + Unless we get a proper cancel() on libzim, that's the only way to not create + a ZIM file on error """ + path, main_page = tmpdir / "test.zim", "welcome" + pycode = f""" +from libzim.writer import Creator +from libzim.writer import Article +class BlobErrorArticle(Article): + def get_data(self): + raise ValueError +zim_creator = Creator("{path}", "{main_page}") +try: + zim_creator.add_article(BlobErrorArticle(**args)) +except Exception: + zim_creator._closed = True +""" + + py = subprocess.run([sys.executable, "-c", pycode]) + assert py.returncode == 0 + assert not path.exists()