835876 - Add the ability to restart downloads

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the jsdownloads API, in some cases, it should be possible restart downloads
that failed or have been canceled.

Comment 1

[:Paolo Amadini]

The "canRestart" property determines whether the download can be restarted after
it is canceled. To restart the download, the "start" method can be used.

Comment 2

[:Paolo Amadini]

Attachment: Work in progress

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.

Comment 3

[Neil Deakin]

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.

Comment 4

[Marco Bonardo [:mak]]

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

Comment 5

[:Paolo Amadini]

Attachment: The patch

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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[:Paolo Amadini]

(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.

Comment 8

[Neil Deakin]

> +      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.

Comment 9

[:Paolo Amadini]

(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.

Comment 10

[:Paolo Amadini]

Attachment: Updated patch

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).

Comment 11

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdfa6a0f2430

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdfa6a0f2430