Thread exception #496

mgautierfr · 2021-02-10T17:13:57Z

~~This is a draft PR, just to track the branch.~~ Now ready to review
Fixes #475
Fixes openzim/python-libzim#153

codecov · 2021-02-10T17:14:07Z

Codecov Report

Base: 84.67% // Head: 84.93% // Increases project coverage by +0.26% 🎉

Coverage data is based on head (b9ac991) compared to base (f26501c).
Patch coverage: 92.98% of modified lines in pull request are covered.

❗ Current head b9ac991 differs from pull request most recent head 76ad28e. Consider uploading reports for the commit 76ad28e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
+ Coverage   84.67%   84.93%   +0.26%     
==========================================
  Files          98       98              
  Lines        4325     4387      +62     
  Branches     1869     1913      +44     
==========================================
+ Hits         3662     3726      +64     
+ Misses        661      660       -1     
+ Partials        2        1       -1

Impacted Files	Coverage Δ
include/zim/writer/creator.h	`100.00% <ø> (ø)`
src/writer/clusterWorker.cpp	`100.00% <ø> (ø)`
src/writer/creatordata.h	`100.00% <ø> (ø)`
src/writer/xapianHandler.h	`100.00% <ø> (ø)`
src/writer/xapianWorker.cpp	`100.00% <ø> (ø)`
src/writer/creator.cpp	`92.40% <82.85%> (+1.72%)`	⬆️
include/zim/error.h	`95.34% <93.54%> (-4.66%)`	⬇️
src/writer/cluster.cpp	`89.56% <100.00%> (+0.37%)`	⬆️
src/writer/clusterWorker.h	`100.00% <100.00%> (ø)`
src/writer/workers.cpp	`100.00% <100.00%> (ø)`
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kelson42 · 2022-01-02T22:20:40Z

@mgautierfr How much work is needed here to complete this PR?

mgautierfr · 2022-08-22T18:22:34Z

Tests are failing because we don't wait enough to have the threads "detecting" the error.
(It is enough on my computer, but on the CI).
I wonder how we should handle that. Simply increase the waiting time or simply don't test that subsequent call are failing (finishZimCreation will always detect the error as it waits for all thread to finish, and it would always be tested) ?

kelson42 · 2022-08-22T19:08:06Z

@mgautierfr All CI does not pass unfortunately. A header/pragma might be missing ;)

mgautierfr · 2022-08-23T09:43:37Z

Indeed, it is usleep on Windows.
But usleep is used to wait for the thread and is related to the problem explain in my previous comment
Better wait for a answer to my question and fix it if needed.

veloman-yunkan

Some of the errors checked by the new unit test come from ASSERTs that are disabled in a release build. This will lead to bug reports similar to The reader test seems to stuck in a deadlock (zim-testing-suite) for some reason. #670 (see The reader test seems to stuck in a deadlock (zim-testing-suite) for some reason. #670 (comment)).
Handling of errors during ZIM creation must be documented.

test/error_in_creator.cpp

src/writer/workers.cpp

src/writer/creator.cpp

test/error_in_creator.cpp

veloman-yunkan

Please rebase/fixup and clean the history of this PR for the next (hopefully, final) review

include/zim/error.h

test/error_in_creator.cpp

kelson42 · 2022-09-05T09:02:05Z

@mgautierfr I have updated to comment to say it "fixes #475". Hope this is correct.

test/error_in_creator.cpp

include/zim/error.h

veloman-yunkan · 2022-09-06T06:25:48Z

The concerns raised in the first review have not been addressed:

Some of the errors checked by the new unit test come from ASSERTs that are disabled in a release build. This will lead to bug reports similar to The reader test seems to stuck in a deadlock (zim-testing-suite) for some reason. #670 (see The reader test seems to stuck in a deadlock (zim-testing-suite) for some reason. #670 (comment)).

Handling of errors during ZIM creation must be documented.

mgautierfr · 2022-09-06T12:52:40Z

Fixed. (I will move the three new commits a bit upper in the git history when rebase-fixup)

veloman-yunkan

There were a few typos in the documentation, but I guess you can fix them as you rebase and fixup.

mgautierfr · 2022-09-28T09:27:52Z

Last push-force split a commit in the history (Make zim::writer::Creator thrown better exception) and create a fixup commit for Introduce CreatorError and InvalidEntry. 4 commits are added at the end. Other commits are unchanged.

veloman-yunkan

Please fix the only last comment, and rebase-fixup for formal approval to merge.

src/writer/clusterWorker.cpp

Creator is based on API the user have to implement. If the user create buggy code or throw exception, we must be prepared to handle them. The new tests show some use cases (all ?) we must be prepared to handle. They are not handle now, will be in next commits.

- Introduce a std::exception_ptr slot to be able to pass a exception from the worker threads to the main thread. - Check the slot on each entry point called from the main thread. - Put the creator "in error" if the exception is set. - Raise a exception if the creator is in error.

…rored

There is no good solution as the value depend of the computer: - Too long and user wait too much for their tests to run - Too short and the tests are failing Let's use a default value and allow user to change the value using a factor defined in WAIT_TIME_FACTOR_TEST env variable.

veloman-yunkan

LGTM. You know why I didn't approve.

This avoid code duplication around waiting_task.

This way we can test that it is the expect exception which is thrown and not another one.

mgautierfr · 2022-10-04T15:03:55Z

Done :)

@veloman-yunkan

* Optimization ofthe first call to `zim::Archive::iterEfficient` (@veloman-yunkan #724) * Add some documentation to `zim::writer::IndexData` (@mgautierfr #727) * Correctly catch and rethrow exception thrown in worker threads at creation (@mgautierfr #496 #748) * Optimization of `Entry::getItem()` (@veloman-yunkan #732) * Fix declaration of `zim::setICUDataDirectory()` (@MohitMaliFtechiz #733) * Add `zim::Archive::getMediatCount()` (@mgautierfr #730) * Make compilaton of examples optional (@mgautierfr #738) * Add a CI for wasm (@mgautierfr #746) * Make constructor of SuggestionItem public (@veloman-yunkan #740)

mgautierfr force-pushed the thread_exception branch from 1748b31 to de4367c Compare February 15, 2021 13:25

mgautierfr force-pushed the thread_exception branch 2 times, most recently from ff82021 to 29d7104 Compare March 2, 2021 14:14

kelson42 added this to the 7.2.0 milestone Dec 4, 2021

mgautierfr force-pushed the thread_exception branch from 29d7104 to 80b962d Compare August 22, 2022 18:09

mgautierfr marked this pull request as ready for review August 22, 2022 18:10

mgautierfr requested a review from veloman-yunkan August 22, 2022 18:10

mgautierfr force-pushed the thread_exception branch from 80b962d to 66964e9 Compare August 22, 2022 18:23

veloman-yunkan requested changes Aug 23, 2022

View reviewed changes

mgautierfr requested a review from veloman-yunkan September 2, 2022 14:04

veloman-yunkan requested changes Sep 4, 2022

View reviewed changes

include/zim/error.h Outdated Show resolved Hide resolved

test/error_in_creator.cpp Outdated Show resolved Hide resolved

test/error_in_creator.cpp Outdated Show resolved Hide resolved

mgautierfr requested a review from veloman-yunkan September 5, 2022 08:32

kelson42 removed this from the 8.2.0 milestone Sep 5, 2022

veloman-yunkan requested changes Sep 5, 2022

View reviewed changes

test/error_in_creator.cpp Outdated Show resolved Hide resolved

test/error_in_creator.cpp Outdated Show resolved Hide resolved

kiwixbot force-pushed the thread_exception branch from 22ab65e to 1e22719 Compare September 5, 2022 13:16

mgautierfr requested a review from veloman-yunkan September 5, 2022 13:16

veloman-yunkan requested changes Sep 6, 2022

View reviewed changes

include/zim/error.h Outdated Show resolved Hide resolved

kiwixbot force-pushed the thread_exception branch from 8c8e8ea to 3d939b9 Compare September 6, 2022 12:55

veloman-yunkan reviewed Sep 6, 2022

View reviewed changes

mgautierfr force-pushed the thread_exception branch 2 times, most recently from 4e95a8e to 44956e9 Compare September 13, 2022 13:35

mgautierfr requested a review from veloman-yunkan September 13, 2022 13:35

mgautierfr force-pushed the thread_exception branch from a2d40fe to 487fabc Compare September 28, 2022 09:25

mgautierfr requested a review from veloman-yunkan September 28, 2022 09:28

veloman-yunkan reviewed Sep 29, 2022

View reviewed changes

src/writer/clusterWorker.cpp Outdated Show resolved Hide resolved

rgaudin mentioned this pull request Oct 3, 2022

Exception in getIndexData crashes interpretor openzim/python-libzim#153

Closed

mgautierfr added 13 commits October 4, 2022 14:26

Introduce CreatorError and InvalidEntry.

907b2ca

Introduce IncoherentImplementationError

42792ec

Only store the first exception.

2afedc7

Use a shared_ptr to store tasks.

b50e4cc

Move the try/catch around all the workers's handler

3d704cb

Make xapianHandler properly not wait for ending task if creator is er…

283f00c

…rored

Add a AsyncError exception to rethrow worker exception.

c0c2995

Throw a CreatorStateError instead of generic runtime_error.

152fe23

Do not try to test potential indexation error if we are without xapian.

ad936a5

Make CI wait a bit more for tests that need to wait for threads.

ace7b67

mgautierfr force-pushed the thread_exception branch from 487fabc to b9ac991 Compare October 4, 2022 12:46

mgautierfr requested a review from veloman-yunkan October 4, 2022 12:47

veloman-yunkan reviewed Oct 4, 2022

View reviewed changes

mgautierfr added 2 commits October 4, 2022 17:03

Introduce a TrackableTask.

8d126e6

This avoid code duplication around waiting_task.

Throw a specific exception in the tests.

76ad28e

This way we can test that it is the expect exception which is thrown and not another one.

mgautierfr force-pushed the thread_exception branch from b9ac991 to 76ad28e Compare October 4, 2022 15:03

veloman-yunkan approved these changes Oct 4, 2022

View reviewed changes

kelson42 merged commit 3a9f574 into master Oct 4, 2022

kelson42 deleted the thread_exception branch October 4, 2022 16:20

mgautierfr mentioned this pull request Nov 30, 2022

New version 8.1.0 #750

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread exception #496

Thread exception #496

mgautierfr commented Feb 10, 2021 •

edited by kelson42

Loading

codecov bot commented Feb 10, 2021 •

edited

Loading

kelson42 commented Jan 2, 2022

mgautierfr commented Aug 22, 2022

kelson42 commented Aug 22, 2022 •

edited

Loading

mgautierfr commented Aug 23, 2022

veloman-yunkan left a comment

veloman-yunkan left a comment

kelson42 commented Sep 5, 2022 •

edited

Loading

veloman-yunkan commented Sep 6, 2022

mgautierfr commented Sep 6, 2022

veloman-yunkan left a comment

mgautierfr commented Sep 28, 2022

veloman-yunkan left a comment

veloman-yunkan left a comment

mgautierfr commented Oct 4, 2022

Thread exception #496

Thread exception #496

Conversation

mgautierfr commented Feb 10, 2021 • edited by kelson42 Loading

codecov bot commented Feb 10, 2021 • edited Loading

Codecov Report

kelson42 commented Jan 2, 2022

mgautierfr commented Aug 22, 2022

kelson42 commented Aug 22, 2022 • edited Loading

mgautierfr commented Aug 23, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

kelson42 commented Sep 5, 2022 • edited Loading

veloman-yunkan commented Sep 6, 2022

mgautierfr commented Sep 6, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Sep 28, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Oct 4, 2022

mgautierfr commented Feb 10, 2021 •

edited by kelson42

Loading

codecov bot commented Feb 10, 2021 •

edited

Loading

kelson42 commented Aug 22, 2022 •

edited

Loading

kelson42 commented Sep 5, 2022 •

edited

Loading