Allow the initial request to take over failed parallel requests.

content/browser/download/download_file_impl.cc

Index: content/browser/download/download_file_impl.cc
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index eb1ef9bfd59e107735c6b49daf8f34df324f1191..4c06e52ee304bdb96a11aa9369a9a9a7de635a9e 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -374,17 +374,7 @@ void DownloadFileImpl::StreamActive(SourceStream* source_stream) {
 
   // Take care of communication with our observer.
   if (reason != DOWNLOAD_INTERRUPT_REASON_NONE) {
-    // Error case for both upstream source and file write.
-    // Shut down processing and signal an error to our observer.
-    // Our observer will clean us up.
-    source_stream->stream_reader()->RegisterCallback(base::Closure());
-    weak_factory_.InvalidateWeakPtrs();
-    SendUpdate();  // Make info up to date before error.
-    std::unique_ptr<crypto::SecureHash> hash_state = file_.Finish();
-    BrowserThread::PostTask(
-        BrowserThread::UI, FROM_HERE,
-        base::Bind(&DownloadDestinationObserver::DestinationError, observer_,
-                   reason, TotalBytesReceived(), base::Passed(&hash_state)));
+    HandleStreamError(source_stream, reason);
   } else if (state == ByteStreamReader::STREAM_COMPLETE || should_terminate) {
     // Signal successful completion or termination of the current stream.
     source_stream->stream_reader()->RegisterCallback(base::Closure());
@@ -515,6 +505,55 @@ bool DownloadFileImpl::IsDownloadCompleted() {
   return true;
 }
 
+void DownloadFileImpl::HandleStreamError(SourceStream* source_stream,
+                                         DownloadInterruptReason reason) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  source_stream->stream_reader()->RegisterCallback(base::Closure());
+  source_stream->set_finished(true);
+
+  bool can_recover_from_error = false;
+
+  if (is_sparse_file_) {
+    auto initial_source_stream = source_streams_.find(save_info_->offset);

[xingliu, 2017/03/18 22:27:34]

What if the error is triggered by the initial stream? can_recover_from_error is
probably set to true. And the file may never be completed.

[qinmin, 2017/03/20 20:59:21]

If the error is trigger by initial stream, set_finished(true) should be called
on line 512. So it will skip the if block below, and can_recover_from_error
should be false.

[xingliu, 2017/03/20 22:33:33]

I see.

[David Trainor- moved to gerrit, 2017/03/21 17:00:16]

Should we just call this stream_iterator or something?  initial_source_stream
and initial_stream are really close in name.

[qinmin, 2017/03/21 19:29:17]

Done.

+    DCHECK(initial_source_stream != source_streams_.end());
+    SourceStream* initial_stream = initial_source_stream->second.get();
+    // If the initial request is alive, check if it can help download all the

[David Trainor- moved to gerrit, 2017/03/21 17:00:16]

Can we add a TODO to figure out allowing adjacent streams to always recover
their neighbors?

[qinmin, 2017/03/21 19:29:17]

Done.

+    // data left by this source stream. We want to avoid the situation that
+    // a server always fail additional requests from the client thus causing the
+    // initial request and the download going nowhere.
+    if (!initial_stream->is_finished() &&
+        (initial_stream->length() == DownloadSaveInfo::kLengthFullContent ||
+         initial_stream->offset() + initial_stream->length() >
+             source_stream->offset() + source_stream->length())) {
+      // The initial stream will download all data downloading from its offset

[xingliu, 2017/03/18 22:27:34]

If there is a new slice added, does the initial slice will have a length cap?

We probably run into this block only when the initial stream fails.

[qinmin, 2017/03/20 20:59:21]

No. For example, there are stream A, B, C ordered by their offset and A is the
initial stream. 
If C fails to write any data, A will check if can download all the data C should
download here. 
If B already starts writing data, then A's length will be truncated. So
can_recover_from_error will return false. Otherwise, A's length is still
kContentlength, so it can help handle C's portion

[xingliu, 2017/03/20 22:33:33]

sgtm.

[David Trainor- moved to gerrit, 2017/03/21 17:00:16]

Hmm that seems like it might be a really common occurrence.  If we start the
requests like:

1. Start A
2. Response from A
3. Start B, then start C
4. Response from B
5. Failure for C.

We just drop the whole request?

Some alternative thoughts:
1. We wait for *all* stream responses to start before truncating the initial
request.  If any fail we fall back to that initial request for the whole
download?
2. We only fail if we've written a large amount of data to B (either > some byte
threshold or > some % of the file).

[qinmin, 2017/03/21 19:29:17]

This will add more complexity to the current logic. I would prefer not doing
this for the initial version.
And later if keep B half open, we don't need to fail the whole request.
DownloadItemImpl will auto resume the download if an interruption occurs here,
so no user action is needed unless the download fails 5 times.

[David Trainor- moved to gerrit, 2017/03/22 03:10:55]

Okay I guess it's fine to postpone this for now.  I just want to make sure we
understand what the typical failure cases are.  Even if we retry automatically,
if the server starts blocking connections the retry could just fail (depending
on how fast we retry) which would be even worse than just having a single
connection to start.

+      // to source_stream->offset(). Close all other streams in the middle.
+      for (auto& stream : source_streams_) {
+        if (stream.second->offset() < source_stream->offset() &&
+            stream.second.get() != initial_stream) {
+          DCHECK_EQ(stream.second->bytes_written(), 0);

[xingliu, 2017/03/18 22:27:34]

Is it safe to assume streams in the middle never write anything?

[qinmin, 2017/03/20 20:59:21]

Yes, if a stream in the middle writes something, it will truncate the length of
all streams prior to it. So those streams will never surpass its starting
offset. 
As a result, the initial request will fail one of the if conditions above.

[xingliu, 2017/03/20 22:33:33]

I see.

[David Trainor- moved to gerrit, 2017/03/21 17:00:16]

Who kills the outstanding request if the middle streams haven't responded yet? 
Does anyone clean up these killed SourceStream objects in this list?

[xingliu, 2017/03/21 17:48:04]

DownloadFileImpl and all objects under it will get killed after
DestinationComplete/DestinationError is called, that the UI thread kill the
DownloadFileImpl with ReleaseDownloadFile or something.

In my understanding, removing SourceStream won't remove the underlying pipe(byte
stream writer+reader).

The pipe will be destory on Job::Cancel, called on interrupt, or the pipe may be
depleted after request is done, and UrlDownloader will remove itself, and also
remove the underlying url request.

If we don't get response for a while, DownloadFileImpl won't create the
SourceStream for it. The url request may still open, not sure if somewhere has
time out logic. That's probably something worthwhile to figure out.

[David Trainor- moved to gerrit, 2017/03/22 03:10:55]

Thanks!

+          stream.second->stream_reader()->RegisterCallback(base::Closure());
+          stream.second->set_finished(true);
+        }
+      }
+      can_recover_from_error = true;
+    }
+  }
+
+  SendUpdate();  // Make info up to date before error.
+
+  if (!can_recover_from_error) {
+    // Error case for both upstream source and file write.
+    // Shut down processing and signal an error to our observer.
+    // Our observer will clean us up.
+    weak_factory_.InvalidateWeakPtrs();

[David Trainor- moved to gerrit, 2017/03/21 17:00:16]

Should we pull out the shutdown code to a common helper?  Do we have these few
lines duplicated elsewhere?

[qinmin, 2017/03/21 19:29:17]

No, we don't have the lines duplicated. It is similar to the DestinationComplete
call, but this one has an interruption reason.
So If we want to move it to a common helper, we would have something like
FinishDownload(bool is_download_complete, int interrupt_reason), which doesn't
feel very straightforward. Or we can simplify it to FinishDownload(int
interrupt_reason). Still, it is not straightforward to figure out what the
function actually does.

[David Trainor- moved to gerrit, 2017/03/22 03:10:55]

Yeah makes sense.  Thanks

+    std::unique_ptr<crypto::SecureHash> hash_state = file_.Finish();
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadDestinationObserver::DestinationError, observer_,
+                   reason, TotalBytesReceived(), base::Passed(&hash_state)));

[xingliu, 2017/03/20 22:33:33]

nit%: Not in this CL, since we allow duplicate IO now, TotalBytesReceived may be
larger than file size. We probably want to

[xingliu, 2017/03/20 22:35:10]

make sure it's ok for user to see larger bytes number than the file size.

[qinmin, 2017/03/21 19:29:17]

This won't happen. TotalBytesReceived is calculated from basefile, and the
DownloadFileImpl::CalculateBytesToWrite() prohibits duplicate write to a
particular file offset. So TotalBytesReceived() should reflect the actual file
size, though it could be different from content-length header.  we didn't
actually record the bytes received from the network in this class. We can get
that in StreamActive() call, but the issue is that they are not stored in db and
not reported to DownloadItemImpl. So only physical bytes written to disk is at
hand.

+  }
+}
+
 DownloadFileImpl::RenameParameters::RenameParameters(
     RenameOption option,
     const base::FilePath& new_path,