search

kiwix / libkiwix

Common code base for all Kiwix ports
https://download.kiwix.org/release/libkiwix/
GNU General Public License v3.0
118 stars 56 forks source link

kiwix::Downloader's constructor pauses all downloads found in the aria session file #1097

Closed veloman-yunkan closed 2 months ago

veloman-yunkan commented 3 months ago

This PR addresses one of the problems observed while working on kiwix/kiwix-desktop#1118. See the commit messages for details.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 32 lines in your changes missing coverage. Please review.

Project coverage is 41.21%. Comparing base (af96b19) to head (65a777d).

Files Patch % Lines
src/aria2.cpp 0.00% 26 Missing :warning:
src/downloader.cpp 0.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1097 +/- ## ========================================== - Coverage 41.45% 41.21% -0.25% ========================================== Files 58 58 Lines 4233 4258 +25 Branches 2316 2332 +16 ========================================== Hits 1755 1755 - Misses 989 1014 +25 Partials 1489 1489 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

veloman-yunkan commented 3 months ago

This is a nice improvement if you have finally found the slow down of all those downloads in kiwix-desktop !!

This PR is a relatively insignificant part of the total effort put into kiwix/kiwix-desktop#1118 - here I address only quite non typical scenarios when kiwix-desktop is terminated with active downloads still running. But since the problem was discovered I couldn't ignore it. And as the fix is self-contained (doesn't depend on anything else) and belongs to a different repository, I filed this PR separately as an appetizer ahead of the main meal bound to be served next week. :smile:

veloman-yunkan commented 3 months ago

Further testing revealed a bug with exception safety of destructors which is now fixed in a fixup commit.

kelson42 commented 3 months ago

@veloman-yunkan @mgautierfr So I guess this PR need new review?

veloman-yunkan commented 3 months ago

@kelson42 An approval was still due, so getting one will now simply require looking at a relatively small change.

kelson42 commented 3 months ago

@veloman-yunkan Great, it is still helpful to reask for review (click on the refresh logo) so there is no ambiguity.

mgautierfr commented 2 months ago

Seems good to me. Please rebase-fixup and merge directly.