Make Downloader threadsafe and more thread compliant. #886
Conversation
Codecov ReportBase: 72.01% // Head: 71.88% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
- Coverage 72.01% 71.88% -0.14%
==========================================
Files 53 54 +1
Lines 3745 3752 +7
Branches 2093 2100 +7
==========================================
Hits 2697 2697
- Misses 1046 1053 +7
Partials 2 2
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. |
There was a problem hiding this comment.
This is the first time I look at the kiwix::Downloader code and, IMO, it will benefit from some extra refactoring/cleanup. Also it might be a good opportunity to replace libcurl with httplib.
However, since this PR immediately addresses a user problem it makes sense to merge it quickly (after the concern about the semantic change in the API is resolved) and discuss the proposed improvements without any haste.
mgautierfr
changed the title
mgautierfr
force-pushed
the
thread_aria
branch
from
4903f79
to
5161477
Compare
As we now build a new request handle for every request, we don't need a lock. libcurl itself is thread safe as long as we don't share a handle.
User may already have a pointer to the `Download` and it is not protected against concurrent access. We could update the status of new created `Download` as by definition, no one have a pointer on it. But it better to not do it neither : - For consistency - Because the first call on update status may be long on windows (because of file preallocation). It is better to not block the downloader for that.
This is dangerous by nature to return raw pointer on internal data.
`false` is a pretty bad default value as most user want to track the real download. By removing the default value, we force user to make a choice. We could have change the default value to true but it would have been a silent API change and we don't want that.
`Waiting` can become `Active` while we are getting the downloads. We may have rare case where we miss a download if we get `Active` before `Waiting`.
mgautierfr
force-pushed
the
thread_aria
branch
from
5161477
to
1ba5882
Compare
Fix #873