Clear callback when a parallel request can no longer write any data

Clear callback when a parallel request can no longer write any data

If a StreamReader is found that it can no longer write any data, we should
clear its callback.
Otherwise, the StreamReader can  still report an error and interrupt the download.

BUG=717719
If the data has already been written

Review-Url: https://codereview.chromium.org/2861443005
Cr-Commit-Position: refs/heads/master@{#469193}
Committed: https://chromium.googlesource.com/chromium/src/+/2a542d8debc1075277dfa8de50dea36fb96d5aff

[qinmin, 2017-05-03 19:45:18]

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

[qinmin, 2017-05-03 19:45:19]

PTAL

[xingliu, 2017-05-03 21:18:33]

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

[commit-bot: I haz the power, 2017-05-03 21:19:08]

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-03 22:10:21]

This cl lgtm.

If any of the sub request has precondition failure, (etag mismatched), the
download is treated as interrupted. So this will cause a higher SERVER_FAIL rate
I think.

Non parallel download will restart the download with If-Range header.

Does it makes sense to ignore precondition fail for sub requests?

[qinmin, 2017-05-03 22:25:46]

On 2017/05/03 22:10:21, xingliu wrote:
> This cl lgtm.
> 
> If any of the sub request has precondition failure, (etag mismatched), the
> download is treated as interrupted. So this will cause a higher SERVER_FAIL
rate
> I think.
> 
> Non parallel download will restart the download with If-Range header.
> 
> Does it makes sense to ignore precondition fail for sub requests?

I think we should keep those, Precondition fail for sub requests will not be
reported by DownloadItemImpl if they are recoverable. So if we saw a high
server_error, I am wondering why precondition fail will cause a non-recoverable
interruption.
It is also better to single out precondition fail from SERVER_ERROR, so that we
can verify it is the root cause of the issue.

[xingliu, 2017-05-03 22:41:17]

On 2017/05/03 22:25:46, qinmin wrote:
> On 2017/05/03 22:10:21, xingliu wrote:
> > This cl lgtm.
> > 
> > If any of the sub request has precondition failure, (etag mismatched), the
> > download is treated as interrupted. So this will cause a higher SERVER_FAIL
> rate
> > I think.
> > 
> > Non parallel download will restart the download with If-Range header.
> > 
> > Does it makes sense to ignore precondition fail for sub requests?
> 
> I think we should keep those, Precondition fail for sub requests will not be
> reported by DownloadItemImpl if they are recoverable. So if we saw a high
> server_error, I am wondering why precondition fail will cause a
non-recoverable
> interruption.
> It is also better to single out precondition fail from SERVER_ERROR, so that
we
> can verify it is the root cause of the issue.

Makes sense, I'll do that.

[commit-bot: I haz the power, 2017-05-03 23:11:28]

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

[commit-bot: I haz the power, 2017-05-03 23:11:29]

Dry run: This issue passed the CQ dry run.

[qinmin, 2017-05-03 23:13:56]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2017-05-03 23:14:41]

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-03 23:20:35]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493853236430710, "parent_rev":
"c76a60b8acefc4672c60799d0a9b5e4d2b242c65", "commit_rev":
"2a542d8debc1075277dfa8de50dea36fb96d5aff"}

[commit-bot: I haz the power, 2017-05-03 23:20:45]

Description was changed from

==========
Clear callback when a parallel request can no longer write any data

If a StreamReader is found that it can no longer write any data, we should
clear its callback.
Otherwise, the StreamReader can  still report an error and interrupt the
download.

BUG=717719

If the data has already been written
==========

to

==========
Clear callback when a parallel request can no longer write any data

If a StreamReader is found that it can no longer write any data, we should
clear its callback.
Otherwise, the StreamReader can  still report an error and interrupt the
download.

BUG=717719

If the data has already been written

Review-Url: https://codereview.chromium.org/2861443005
Cr-Commit-Position: refs/heads/master@{#469193}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a542d8debc1075277dfa8de50de...
==========

[commit-bot: I haz the power, 2017-05-03 23:20:46]

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