Closed Bug 835876 Opened 7 years ago Closed 7 years ago

Add the ability to restart downloads

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

In the jsdownloads API, in some cases, it should be possible restart downloads
that failed or have been canceled.
The "canRestart" property determines whether the download can be restarted after
it is canceled. To restart the download, the "start" method can be used.
Summary: Add the ability to resume downloads → Add the ability to restart downloads
Attached patch Work in progress (obsolete) — Splinter Review
This is an early version of the patch, I still have to re-read it and add
comments to explain the internal code flow. The function comments, though,
should already explain the cancel and restart logic, as seen from the outside.

Note that the "cancel" method now returns a promise. Knowing when cancellation
finished will be useful when shutting down the browser, to allow downloads to
stop cleanly before serializing the download list.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #714973 - Flags: feedback?(mak77)
Attachment #714973 - Flags: feedback?(enndeakin)
Comment on attachment 714973 [details] [diff] [review]
Work in progress

> function Download()
> {
>-  this._deferStopped = Promise.defer();
>+  this._deferSucceeded = Promise.defer();

You rename this here but don't rename it elsewhere.

>-   * Starts the download.
>+   * Starts the download for the first time, or restarts a download that failed
>+   * or has been canceled.
>+   *
>+   * Calling this method when the download is in progress or has been completed
>+   * successfully has no effect, and the method returns a resolved promise.
>+   *
>+   * If the "cancel" method was called but the cancellation process has not
>+   * finished yet, this method waits for the cancellation to finish, then
>+   * restarts the download immediately.

I'd also document that to start a download fresh, you need to create a new Download object.
Attachment #714973 - Flags: feedback?(enndeakin) → feedback+
Comment on attachment 714973 [details] [diff] [review]
Work in progress

Review of attachment 714973 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +208,5 @@
> +      deferStopped.resolve(Task.spawn(function task_D_start() {
> +        try {
> +          if (this._deferCanceled) {
> +            yield this._deferCanceled.promise;
> +          }

doesn't look like this must be inside the try

@@ +215,5 @@
> +          this.succeeded = true;
> +          this._deferSucceeded.resolve();
> +        } catch (ex) {
> +          if (this._deferCanceled) {
> +            if (this._deferStopped === deferStopped || !this._deferStopped) {

This needs some comment

@@ +225,5 @@
> +          throw ex;
> +        } finally {
> +          let deferCanceled = this._deferCanceled;
> +          this._deferCanceled = null;
> +          if (this._deferStopped === deferStopped || !this._deferStopped) {

also here. more globally the logic needs some inline documentation
Attachment #714973 - Flags: feedback?(mak77) → feedback+
Attached patch The patch (obsolete) — Splinter Review
This version is updated with the final logic, and with all the detailed test
cases to ensure that download state is consistent regardless of the method you
use to wait for completion or cancellation.
Attachment #714973 - Attachment is obsolete: true
Attachment #717517 - Flags: review?(enndeakin)
Attachment #717517 - Flags: feedback?(mak77)
Comment on attachment 717517 [details] [diff] [review]
The patch

Review of attachment 717517 [details] [diff] [review]:
-----------------------------------------------------------------

Following all of the logic is starting becoming complicated, with all of these replaces promises and partial patches...
Unless I missed them, I couldn't see tests for API misuses, like invoking start() or cancel() multiple times in a row, rather than interleaved. Or any other tests based on what you think people may do with the API that is unexpected.
Attachment #717517 - Flags: feedback?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] (Away Mar 1) from comment #6)
> I couldn't see tests for API misuses, like invoking
> start() or cancel() multiple times in a row, rather than interleaved

I forgot to add a test for that. I'll do it, since that is not exactly an API
misuse, but could rather be a race condition from which the API is designed
to be protected.

Other accidental misuses (like for example passing integers instead of objects)
are out of scope for now, because there are really too many of them that could
be thought of. We can add barriers later if we see it's worth it.
> +      let deferCanceled = Promise.defer();
> +      this._currentAttempt.then(function () deferCanceled.resolve(),
> +                                function () deferCanceled.resolve());

Shouldn't the second argument be deferCanceled.reject()?


+add_task(function test_download_cancel_midway()
...
+      if (!download.stopped && !download.canceled && download.progress == 50) {
+        deferCancel.resolve(download.cancel());
+
+        // The state change happens immediately after calling "cancel", but
+        // temporary files or part files may still exist at this point.
+        do_check_true(download.canceled);

Do you want to check to ensure that download.stopped isn't changed when cancelling? Or do you not know for sure?


+add_task(function test_download_cancel_midway_restart()
...
+  // Allow the second request to complete.
+  interruptible.resolve();

This is a bit confusing. 'interruptible' refers to the first request.

For later, you might also consider making the download properties readonly.
(In reply to Neil Deakin from comment #8)
> > +      let deferCanceled = Promise.defer();
> > +      this._currentAttempt.then(function () deferCanceled.resolve(),
> > +                                function () deferCanceled.resolve());
> 
> Shouldn't the second argument be deferCanceled.reject()?

Even if the download completes successfully instead of being canceled (because
of a race condition) the promise returned by cancel() is never rejected.

> +add_task(function test_download_cancel_midway()
> ...
> +      if (!download.stopped && !download.canceled && download.progress ==
> 50) {
> +        deferCancel.resolve(download.cancel());
> +
> +        // The state change happens immediately after calling "cancel", but
> +        // temporary files or part files may still exist at this point.
> +        do_check_true(download.canceled);
> 
> Do you want to check to ensure that download.stopped isn't changed when
> cancelling? Or do you not know for sure?

We don't know for sure at this point.

> +add_task(function test_download_cancel_midway_restart()
> ...
> +  // Allow the second request to complete.
> +  interruptible.resolve();
> 
> This is a bit confusing. 'interruptible' refers to the first request.

I agree this can be confusing. Does bug 835803 solve the ambiguity?

> For later, you might also consider making the download properties readonly.

Yes, we can evaluate this in later phases.
Attached patch Updated patchSplinter Review
Neil, let me know if you're satisfied with the answers in comment 9,
considering that the clarification on "interruptible.reject" is handled
by the changes in bug 835803 (if you think it requires further clarifications,
feel free to leave a comment in that bug).
Attachment #717517 - Attachment is obsolete: true
Attachment #717517 - Flags: review?(enndeakin)
Attachment #720412 - Flags: review?(enndeakin)
Attachment #720412 - Flags: review?(enndeakin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdfa6a0f2430
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/bdfa6a0f2430
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.