Reduce unnecessary download interruptions due to parallel requests

Reduce unnecessary download interruptions due to parallel requests

If a parallel request fails to when response starts, it will interrupt the download.
However, the failure may be recoverable by the main requst.
And this issue may cause a download to never finish even if user resumes it.
This change fixes the issue by allowing the failure to passthrough to DownloadFileImpl.
So that DownloadFileImpl can check whether the failure is recoverable.
A side effect of this change is more we will create received_slice vector even if all parallel requests fails.
This can be addressed in later changes.

BUG=720057
Review-Url: https://codereview.chromium.org/2872943003
Cr-Commit-Position: refs/heads/master@{#470994}
Committed: https://chromium.googlesource.com/chromium/src/+/c41a4fc09f9356d7d225747454b707eccd745e54

[qinmin, 2017-05-09 18:59:07]

qinmin@chromium.org changed reviewers:
+ dtrainor@chromium.org, xingliu@chromium.org

[qinmin, 2017-05-09 18:59:08]

PTAL

[qinmin, 2017-05-09 18:59:13]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 19:00:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 19:23:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:23:31]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[xingliu, 2017-05-10 01:40:17]

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:448:
DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE) {
Are errors for non-parallel download also affected by this change?

And do we plan to handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED in this CL?
that's the error we've seen higher in the metric.

If we handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED here, does it also affect
non-parallel downloads? 

In the long run, shall we assess the way we send errors around in
content/download to see if it makes sense to centralize the error piping around
the byte stream?

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
File content/browser/download/download_worker.cc (right):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_worker.cc:18: CompletedByteStreamReader(int
status) : status_(status){};
nit: space between ) and {}.

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_worker.cc:27: void RegisterCallback(const
base::Closure& sink_callback) override{};
nit: also space here.

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.cc (left):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:140:
SetPotentialFileLength(worker->offset());
nit: Shall we remove DownloadJob::SetPotentialFileLength and
DownloadJob::Interrupt? since all errors now go through DownloadFile.

[qinmin, 2017-05-10 17:47:24]

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
File content/browser/download/download_file_impl.cc (right):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_file_impl.cc:448:
DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE) {
On 2017/05/10 01:40:17, xingliu wrote:
> Are errors for non-parallel download also affected by this change?
> 
> And do we plan to handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED in this CL?
> that's the error we've seen higher in the metric.
> 
> If we handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED here, does it also affect
> non-parallel downloads? 
> 
> In the long run, shall we assess the way we send errors around in
> content/download to see if it makes sense to centralize the error piping
around
> the byte stream?

For non-parallel download, if we receive
DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE, the current behavior is to restart
from the beginning. This is different from the parallel download behavior
because the parallel request can start from the end. Making the if statement
more restricted so it only applies to the request after the last received slice.

We don't need to special handle DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED. that's
only for UMA purpose, I don't think it makes too much sense to single out a
specific error reason for special handing while it can be treated the same way
as the remaining errors.
There are several different errors: file error, server error and connection
error. DownloadFileImpl reports file errors, while the other errors need to be
passed down to here through thread hoppings. So if we want to centralize all
errors, we need to figure out which thread will be responsible for handling
this. and how are we going to pass those errors around. I don't think this is a
top priority thing to do, but maybe for long term refactoring purpose.

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
File content/browser/download/download_worker.cc (right):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_worker.cc:18: CompletedByteStreamReader(int
status) : status_(status){};
On 2017/05/10 01:40:17, xingliu wrote:
> nit: space between ) and {}.

wierd, this is the result of "git cl format". Fixed anyway.

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/do...
content/browser/download/download_worker.cc:27: void RegisterCallback(const
base::Closure& sink_callback) override{};
On 2017/05/10 01:40:17, xingliu wrote:
> nit: also space here.

Done.

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/pa...
File content/browser/download/parallel_download_job.cc (left):

https://codereview.chromium.org/2872943003/diff/1/content/browser/download/pa...
content/browser/download/parallel_download_job.cc:140:
SetPotentialFileLength(worker->offset());
On 2017/05/10 01:40:17, xingliu wrote:
> nit: Shall we remove DownloadJob::SetPotentialFileLength and
> DownloadJob::Interrupt? since all errors now go through DownloadFile.

Done.

[qinmin, 2017-05-10 17:47:32]

The CQ bit was checked by qinmin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 17:48:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xingliu, 2017-05-10 18:11:33]

lgtm

[David Trainor- moved to gerrit, 2017-05-10 20:12:49]

lgtm

[commit-bot: I haz the power, 2017-05-10 20:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:50:24]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[qinmin, 2017-05-10 21:20:43]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:22:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 03:27:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 03:27:20]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[qinmin, 2017-05-11 17:15:40]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-05-11 17:16:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 17:24:27]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494522940119120,
"parent_rev": "b743d73b67249433eead4af1f526d77acc31e0cc", "commit_rev":
"952a0565e328026442d4d9a4b3dc0b61297993f4"}

[commit-bot: I haz the power, 2017-05-11 17:24:43]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494522940119120,
"parent_rev": "15c51743b4ca6540336772cf1ac33bdc16b97389", "commit_rev":
"c41a4fc09f9356d7d225747454b707eccd745e54"}

[commit-bot: I haz the power, 2017-05-11 17:24:53]

Description was changed from

==========
Reduce unnecessary download interruptions due to parallel requests

If a parallel request fails to when response starts, it will interrupt the
download.
However, the failure may be recoverable by the main requst.
And this issue may cause a download to never finish even if user resumes it.
This change fixes the issue by allowing the failure to passthrough to
DownloadFileImpl.
So that DownloadFileImpl can check whether the failure is recoverable.
A side effect of this change is more we will create received_slice vector even
if all parallel requests fails.
This can be addressed in later changes.

BUG=720057
==========

to

==========
Reduce unnecessary download interruptions due to parallel requests

If a parallel request fails to when response starts, it will interrupt the
download.
However, the failure may be recoverable by the main requst.
And this issue may cause a download to never finish even if user resumes it.
This change fixes the issue by allowing the failure to passthrough to
DownloadFileImpl.
So that DownloadFileImpl can check whether the failure is recoverable.
A side effect of this change is more we will create received_slice vector even
if all parallel requests fails.
This can be addressed in later changes.

BUG=720057

Review-Url: https://codereview.chromium.org/2872943003
Cr-Commit-Position: refs/heads/master@{#470994}
Committed:
https://chromium.googlesource.com/chromium/src/+/c41a4fc09f9356d7d225747454b7...
==========

[commit-bot: I haz the power, 2017-05-11 17:24:55]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/c41a4fc09f9356d7d225747454b7...