Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception inside contextmanager should cancel the zim creation #42

Open
rgaudin opened this issue Jun 10, 2020 · 4 comments
Open

Exception inside contextmanager should cancel the zim creation #42

rgaudin opened this issue Jun 10, 2020 · 4 comments
Labels
enhancement New feature or request upstream
Milestone

Comments

@rgaudin
Copy link
Member

rgaudin commented Jun 10, 2020

When using the context-manager, should a (non libzim) error occur, the exception is raised but the finalization is done on the Creator as if everything went well.

with libzim.writer.Creator(
    "test_x07.zim", main_page="A/index.html", index_language="eng", min_chunk_size=2048,
) as zfile:
    zfile.add_article(DumbArticle("index.html", "hello", ARTICLE_MIME, "bonjour"))
    raise Exception("outch")
    zfile.add_article(DumbArticle("page2.html", "hello2", ARTICLE_MIME, "bonjour2"))

T:0; A:4; RA:0; CA:4; UA:0; FA:0; IA:1; C:0; CC:0; UC:0; WC:1
T:0; Waiting for workers
T:0; ResolveRedirectIndexes
Resolve redirect
T:0; Set article indexes
set index
T:0; Resolve mimetype
T:0; create title index
T:0; 6 title index created
T:0; 2 clusters created
T:0; write zimfile :
T:0;  write mimetype list
T:0;  write directory entries
T:0;  write url prt list
T:0;  write title index
T:0;  write cluster offset list
T:0;  write header
T:0;  write checksum
T:0; rename tmpfile to final one.
T:0; finish
Traceback (most recent call last):
  File "./demo.py", line 55, in <module>
    raise Exception("outch")
Exception: outch

This results in a valid ZIM file on the filesystem but lacking the second article of course.

I think the expected behavior would be to cancel the ZIM creation and remove temporary files.

@mgautierfr @kelson42 ?

@rgaudin rgaudin added enhancement New feature or request question Further information is requested labels Jun 10, 2020
@mgautierfr
Copy link
Collaborator

Yes. However, there is no proper way to cancel the zim creation on libzim side itself.

I've just open a issue on libzim side : openzim/libzim#347

@rgaudin rgaudin added upstream and removed question Further information is requested labels Jun 10, 2020
@rgaudin rgaudin changed the title Shouldn't exception inside the contextmanager cancel the zim creation? exception inside contextmanager should cancel the zim creation Jun 10, 2020
@kelson42 kelson42 pinned this issue Jul 4, 2020
@rgaudin rgaudin mentioned this issue Feb 1, 2021
@rgaudin
Copy link
Member Author

rgaudin commented Feb 4, 2021

@mgautierfr, I tested the thread_exception branch on libzim but I see no difference in how an exception in a ContentProvider is handled"

def test_creator_exception_item(fpath):
    class AContentProvider:
        def get_size(self):
            return 1

        def feed(self):
            raise FileNotFoundError("missing file")

    class AnItemWithCP(StaticItem):
        def get_contentprovider(self):
            return AContentProvider()

    with Creator(fpath) as c:
        with pytest.raises(RuntimeError, match="FileNotFoundError"):
            c.add_item(AnItemWithCP())

It crashes with an output similar to:

Resolve redirect
set index
libc++abi.dylib: terminating with uncaught exception of type std::runtime_error: Traceback (most recent call last):
  File "libzim/wrapper.pyx", line 127, in libzim.wrapper.blob_cy_call_fct
    blob = func()
  File "/Users/reg/src/pylibzim/tests/test_libzim_creator.py", line 132, in feed
    raise FileNotFoundError("missing file")
FileNotFoundError: missing file

Fatal Python error: Aborted

What's different though is that a lot of the tests are now failing with the obscure RuntimeError: Creator is in error state yet still crashing the process.

@kelson42 kelson42 changed the title exception inside contextmanager should cancel the zim creation Exception inside contextmanager should cancel the zim creation Feb 10, 2021
@stale
Copy link

stale bot commented Oct 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Oct 2, 2021
@kelson42 kelson42 added this to the 1.3.0 milestone May 7, 2022
@stale stale bot removed the stale label May 7, 2022
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jul 10, 2022
@kelson42 kelson42 modified the milestones: 2.1.0, 2.2.0 Sep 28, 2022
@stale stale bot removed the stale label Sep 28, 2022
@kelson42 kelson42 modified the milestones: 2.2.0, 3.1.0 Apr 6, 2023
@kelson42 kelson42 modified the milestones: 3.1.0, 3.2.0 Apr 27, 2023
@kelson42 kelson42 modified the milestones: 3.2.0, 3.3.0 Nov 1, 2023
@kelson42 kelson42 modified the milestones: 3.4.0, 3.5.0 Dec 3, 2023
@kelson42 kelson42 added this to the 3.6.0 milestone Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request upstream
Projects
None yet
Development

No branches or pull requests

3 participants