Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download Service Work #484

Open
ahmedre opened this issue Mar 8, 2015 · 4 comments
Open

Download Service Work #484

ahmedre opened this issue Mar 8, 2015 · 4 comments

Comments

@ahmedre
Copy link
Contributor

ahmedre commented Mar 8, 2015

QuranDownloadService is currently one download at a time. while this is intentional, it is also problematic:

  • we artificially block the user while downloading the initial set of pages, though there's no reason we actually have to do that (other than fear of them trying to download other stuff while the pages are downloading). see make initial download non-blocking #415.
  • the new AudioManager also blocks (for the same reason).
  • today, attempting to download a large sura, then heading into translations and tapping a translation to download does absolutely nothing.

proposed solution

  • 4 lazily initialized SingleThreadExecutors - for pages, audio, translations, and "mass audio," respectively.
  • intent is passed to one of those where it stays in the queue
  • ui thread Handler can take "done" posts to stopSelf when everything is done (or the thread itself can call stopSelf after locking and decrementing an int and ensuring it's 0).
  • need to figure out a good way to handle cancellation.
  • need to figure out how to handle the same file being requested multiple times (ex start downloading all the suras for a sheikh, and attempting to play a not-yet-downloaded sura for the same sheikh).

alternative solution

  • don't block downloads at the ui level
  • effectively communicate "only one download at a time"

while the alternative solution is better for cost (server load, etc), the proposed solution above is better for usability. in practice (after the first initial download), a user would still only be able to have 2 long running downloads at a time (audio download from the Quran pages and one from the download manager).

@ahmedre
Copy link
Contributor Author

ahmedre commented May 25, 2015

more thoughts:

  • need a way to distinguish request types (tag when using OkHttp).
  • need a way to only update range if file hasn't changed (If-Unmodified-Since flag, catch the 412 and delete the partial file and start over).

@ahmedre
Copy link
Contributor Author

ahmedre commented May 25, 2015

regarding the above comment, i spent a good bit of time playing with this today, and wanted to document my findings here for future purposes.

to put this in context, this is trying to figure out a good way to solve the following problem - since we allow downloading large files, we allow the user to continue the download from where they left off. so what happens if the file changes server side? since this doesn't happen often, our way of dealing with this up until now was to either pretend the problem doesn't exist (with audio, for example), or rename or move the pages and clear any partial downloads (as was done to fix #529, for example).

there are 2 ways of fixing this problem:

  1. If-Range - this is the recommended way of doing things, since essentially, it tells the server, "hey, if this file is the same one i started downloading from, resume (returning a 206), otherwise, start all over (returning a 200)." If-Range takes either an ETag or a last modified date. however, since, as of today, the file last modified times are not necessarily the same across servers (mainly due to not transferring the files with a flag that preserves these dates), we can't rely on this yet. note that nginx uses a combination of last modified date and content length for its ETag, and lighttpd does something different. while we are mostly on nginx, we still have one server on lighttpd that needs to be migrated as well before this is a consideration (even if the dates were fixed across all files).
  2. If-Unmodified-Since - this basically says, "if the file has been modified after this date, throw an error (412), otherwise, resume downloading the file." the main problem with this (aside from the fact that its potentially 2 http requests versus the above which is just 1) is that it doesn't work on lighttpd (it ignores this header and just continues serving the range without checking). a secondary problem is that because, until now, the Last-Modified field is different on the servers, this may still not work (if one downloads 50% from server 1 with a Last-Modified date x, and then tried to continue from server 2 with Last-Modified of x + 1, it would think the file is invalidated and would restart the download).

one workaround would be to add an offset (ex 1 day) to the Last-Modified header and use that as the flag to If-Unmodified-Since.

that having been said, the two solutions would be:

  1. ensure consistency across all last modified timestamps between all the servers and implement solution 1 client side (using Etag if all the servers are on nginx, or Last-Modified otherwise).
  2. ensure "nearness" across all last modified timestamps between the servers (i.e. ensure a delta of 1 day or less), migrate the last server to nginx, and then implement solution 2.

either way, this is likely not an extremely high priority feature at this point in time.

@ahmedre ahmedre closed this as completed May 25, 2015
@ahmedre ahmedre reopened this May 25, 2015
ahmedre added a commit that referenced this issue May 25, 2015
This new directory contains the newest image zips. For now, did not want
to change the existing zips in place, since this can cause incorrect
downloads (using part of an old file and continuing with part of the new
one). Eventually, this will be inevitable, since we're likely to symlink
the old files to the new ones at some point (so that the many users who
have not yet upgraded can still get the newest images if downloading
their images for the first time).

Also see #484 for more discussions around this problem.
ahmedre added a commit that referenced this issue Aug 9, 2015
This new directory contains the newest image zips. For now, did not want
to change the existing zips in place, since this can cause incorrect
downloads (using part of an old file and continuing with part of the new
one). Eventually, this will be inevitable, since we're likely to symlink
the old files to the new ones at some point (so that the many users who
have not yet upgraded can still get the newest images if downloading
their images for the first time).

Also see #484 for more discussions around this problem.
ahmedre added a commit that referenced this issue Aug 9, 2015
This new directory contains the newest image zips. For now, did not want
to change the existing zips in place, since this can cause incorrect
downloads (using part of an old file and continuing with part of the new
one). Eventually, this will be inevitable, since we're likely to symlink
the old files to the new ones at some point (so that the many users who
have not yet upgraded can still get the newest images if downloading
their images for the first time).

Also see #484 for more discussions around this problem.
ahmedre added a commit that referenced this issue Oct 14, 2017
This new directory contains the newest image zips. For now, did not want
to change the existing zips in place, since this can cause incorrect
downloads (using part of an old file and continuing with part of the new
one). Eventually, this will be inevitable, since we're likely to symlink
the old files to the new ones at some point (so that the many users who
have not yet upgraded can still get the newest images if downloading
their images for the first time).

Also see #484 for more discussions around this problem.
@murtraja
Copy link
Contributor

#498 point 3, is it related to this?

@ahmedre
Copy link
Contributor Author

ahmedre commented Mar 19, 2019

wow, this issue is super old!
i've written a download manager that supports multiple downloads that i use in other apps - i want to incorporate it here at some point in the near future since it will fix most of the problems mentioned in the first point.

#498's point 3 is still valid though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants