Asynchronous execution of download actions #1118
Conversation
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
f840241
to
60f0817
Compare
|
@veloman-yunkan This is an attempt to fix #916 ? |
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
318edc0
to
a7b5cde
Compare
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
3d0e8b4
to
cb8dc25
Compare
veloman-yunkan
force-pushed
the
robust_download_management
branch
2 times, most recently
from
9a97f69
to
25b66e6
Compare
veloman-yunkan
changed the title
|
@ShaopengLin @sgourdas I invite you to look at this PR as a educational material on refactoring. It illustrates how a major change can be made incrementally by carrying out significant amount of groundwork that facilitates implementing the sought goal. Please also feel free to do a real review and provide feedback! |
One known issue with this PR is that it is very big. But breaking it up into smaller ones is possible upon request. Yet it is better that the reviewers can see the full picture and can participate in deciding if and how the decomposition should be performed. |
|
@kelson42 Can you please perform some independent testing? I did it by downloading to a KIWIX/WIKIMEDIA branded USB stick from the hackathon. Note that |
There was a problem hiding this comment.
Nice PR. I have seen only few changes.
On functional side, now when closing kiwix-desktop with a ongoing download, at restart the download is paused. Would be nice to have it automatically restarted (especially as a downloading zim is not visible in local library, the default view).
But can be made in another PR, this one is big enough.
src/downloadmanagement.cpp
Outdated
| @@ -82,7 +82,7 @@ DownloadManager::~DownloadManager() | |||
|
|
|||
| // At this point the thread may be stuck waiting for data. | |||
| // Let's wake it up. | |||
| m_requestQueue.enqueue(""); | |||
| m_requestQueue.enqueue({UPDATE, ""}); | |||
There was a problem hiding this comment.
Would it be better to have a PING action instead of using the (empty) id ?
There was a problem hiding this comment.
The id component will still need to be provided, so it won't look much prettier here.
There was a problem hiding this comment.
Maybe hide it in a wake_up or notify method ?
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
25b66e6
to
cb6b6fc
Compare
|
The changes were inserted as fixup commits at the correct locations in the PR history. |
Agree. There are also a couple of edge cases like the book appearing in an obscure state when the app exits while the download request has been enqueued but not yet submitted to aria2 (this is easily cleared via the "Delete book" action but may be confusing to the user). I think that those too better be fixed in a separate PR. |
veloman-yunkan
force-pushed
the
robust_download_management
branch
from
cb6b6fc
to
529c0ca
Compare
|
@veloman-yunkan I have tested briefly and it works better than before and I was not able to detect any bug... but can you please fix so "Download" and the downloading progression are horizontaly center in the column? They are not (anymore?) |
Let's first merge this PR, and we can pursue purely aesthetic issues afterwards. |
|
There is one problem: if i click on download, it does not download straight. It should. |
|
I have just answered on your answer of my comment (comment of comment of comment...) At least, you can rebase-fixup before a final approval. |
That is the root cause of the PR. Sometime aria2 takes time to start the download. How much time is "not download straight" ? |
If download info becomes stale (due to slow responses from aria)
the download speed is shown as ---.
Known issues:
- A stale download is refreshed only due to a GUI event (such
as scrolling or mouse hover).
Made the download updater thread process requests from a queue. Such a scheme paves the path to performing all download actions asynchronously by placing them onto the queue (which will eliminate non-responsiveness of the application when those commands hit the problem with slow responses from aria2c).
Pausing a download now changes its state to PAUSE_REQUESTED and the pause request is enqueued for asynchronous execution. Similarly, resuming a download changes its state to RESUME_REQUESTED and the resume request is enqueued for asynchronous execution. In the PAUSE_REQUESTED and RESUME_REQUESTED states download actions are disabled. Known issues: - The PAUSE_REQUESTED state may be incorrectly restored to DOWNLOADING if at the time of pausing an earlier UPDATE request for the same download preceded it in the queue or was being executed. After the response to the said UPDATE request is received it results in the DownloadState being reset to the previous state. But after the pause request is processed a subsequent UPDATE request will change the state to PAUSED. In GUI this looks as follows (assuming slow responses from aria2c, allowing to observe the events in slow motion): 1. Pause button is pressed 2. Pause button and download progress info (the textual one) disappear 3. Pause button and download progress info (the textual one) re-appear 4. Download switches to paused state without any further user actions A similar problem exists for resuming the download. The solution is to convert the request queue to a priority queue where download actions are given precedence over update actions. However this will not eliminate the problem completely since a pause/resume action may be issued while an update request is being processed by aria2c (the likelyhood of which greatly increases if aria is mostly stuck struggling with slow storage). The latter case will be addressed by tracking the timestamps of the requests and ignoring the download status from those update requests that were issued before the user action requests.
During the previous commit indentation of some lines was preserved in order to minimize the diff. Now it is fixed.
... provided that download actions are inserted into the queue *before* existing update requests.
Separated checks that can be performed early (independent of aria2c services). When the download initiation is converted to a 2-stage procedure (similar to how it was done for pausing/resuming/cancelling a download) those checks better be performed synchronously in the main thread.
Extracted from ContentManager::downloadBook() the part that should be executed asynchronously into a new function `ContentManager::startDownload()`.
Preparing to handle errors originating in the (future) asynchronous execution of `ContentManager::startDownload()`.
kelson42
force-pushed
the
robust_download_management
branch
from
45d953c
to
2a1d53f
Compare
|
Rebasing to benefit of the CI fix |
I have open #1145 |
|
@veloman-yunkan @mgautierfr Are we good to merge?! Beside a discussion still open I see nothing left but not formal approval either?! |
Should fix #1094.
This PR solves issues caused by the fact that aria2c is a single threaded application (processing incoming data, including RPC requests, using the polled I/O approach) and it becomes unresponsive when saving to slow storage (such as old USB sticks).
The solution is based on issuing all (with one exception, see footnote ¹) requests to aria2c asynchronously, on a separate working thread. Before this PR only download status updates were processed asynchronously. Now download actions (start, pause, resume, cancel) are also executed asynchronously.
Now, after the user initiates a download action (for example, by clicking the download button) all download-related actions for that book are disabled until the response to that request is received.
The PR starts with a lot of refactoring, consolidating most of the download-related code in a separate pair of h/cpp source files.
It also contains a few minor otherwise unrelated fixes/improvements.
This PR depends on kiwix/libkiwix#1097 - mainly through some code that was moved from
kiwix-desktoptolibkiwix, though that part could in principle be extracted into a separate PR that may be merged later on.¹ The only place where aria2c requests are executed in the main thread is during the creation of
kiwix::Downloader. That loophole is worked around by kiwix/libkiwix#1097 (on which this PR also depends in one of the last commits) - all downloads are paused by modifying the aria2 session file so that aria2c stays idle and thus responsive.