kiwix::Downloader's constructor pauses all downloads found in the aria session file #1097
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
| if ( !startsWith(line, " pause=") ) { | ||
| ss << line << "\n"; | ||
| } | ||
| if ( !line.empty() && line[0] != ' ' && line[0] != '#' ) { |
There was a problem hiding this comment.
With line[0] != ' ', you are checking that line doesn't start with ' ',
but " pause=" is starting by a space.
Is there something wrong ?
There was a problem hiding this comment.
No, everything is correct. The aria2c input file can contain a list of URIs with options specified after each line of URIs (these optional lines must start with white space(s)). Lines starting with # are treated as comments and skipped.
In other words, a commentless aria2 input file looks like below:
URI1
option11
option12
...
URI2
option21
option22
...
The expression under question evaluates to true (only) for URI lines. As soon as such a line is read (and output by the preceding statement) a pause=true option is appended after it.
src/aria2.cpp
Outdated
| MethodCall methodCall("aria2.pauseAll", m_secret); | ||
| doRequest(methodCall); | ||
|
|
There was a problem hiding this comment.
Do we really need the first commit adding this ?
There was a problem hiding this comment.
I had the same question, and I chose to keep this change because it contributes to a more gradual way of explaining the problem and leading to (and justifying) the most robust solution.
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 |
|
Further testing revealed a bug with exception safety of destructors which is now fixed in a fixup commit. |
|
@veloman-yunkan @mgautierfr So I guess this PR need new review? |
|
@kelson42 An approval was still due, so getting one will now simply require looking at a relatively small change. |
|
@veloman-yunkan Great, it is still helpful to reask for review (click on the refresh logo) so there is no ambiguity. |
|
Seems good to me. Please rebase-fixup and merge directly. |
Otherwise, creating a Downloader object next time may take very long (or that operation may get stuck) if an active download is being saved to slow media.
Downloader constructor may get stuck if the check for the aria2c RPC being up gets stuck due to curl_easy_perform() never returning (or, at least, taking longer than I was willing to wait). Currently it may happen, for example, after an application crashes with active downloads being saved to slow media. Then the next creation of a Downloader object will deal with aria2c immediately resuming those downloads and becoming unresponsive as it struggles flushing incoming data to slow storage (or because of some other unfortunate timing of the RPC request being received while it cannot yet be served).
The aria session file is edited before starting aria2c. This ensures responsive aria2c RPC server even after a crash.
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
7c135c1
to
65a777d
Compare
This PR addresses one of the problems observed while working on kiwix/kiwix-desktop#1118. See the commit messages for details.