Handle early pause, cancel for parallel requests.

content/browser/download/parallel_download_job.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/parallel_download_job.h"
 
 #include "base/memory/ptr_util.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/parallel_download_utils.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/storage_partition.h"
 
 namespace content {
+namespace {
+
+const int kVerboseLevel = 1;
+
+}  // namespace
 
 ParallelDownloadJob::ParallelDownloadJob(
     DownloadItemImpl* download_item,
     std::unique_ptr<DownloadRequestHandleInterface> request_handle,
     const DownloadCreateInfo& create_info)
     : DownloadJobImpl(download_item, std::move(request_handle)),
       initial_request_offset_(create_info.offset),
       content_length_(create_info.total_bytes),
       requests_sent_(false) {}
 
@@ skipping 54 matching lines @@
   DCHECK(!requests_sent_);
   DCHECK(!timer_.IsRunning());
 
   timer_.Start(FROM_HERE, GetParallelRequestDelayConfig(), this,
                &ParallelDownloadJob::BuildParallelRequests);
 }
 
 void ParallelDownloadJob::OnByteStreamReady(
     DownloadWorker* worker,
     std::unique_ptr<ByteStreamReader> stream_reader) {
-  DownloadJob::AddByteStream(std::move(stream_reader), worker->offset(),
-                             worker->length());
+  // Destroy the request if user canceled.
+  if (DownloadJob::is_canceled()) {
+    VLOG(kVerboseLevel) << "Byte stream arrived after user cancel the request.";
+    worker->Cancel();

[qinmin, 2017/03/21 22:53:55]

do you need to do this? If cancel is called before OnByteStreamReady, shouldn't
worker->cancel() already been called in ParallelDownloadJob::Cancel()?

I think you can just early return here?

[xingliu, 2017/03/21 23:19:14]

At that time the request handle is not set into the worker. In
OnUrlDownloaderStarted the request handle and the byte stream reader is passed
in after response is handled.

So we either let worker know more states and defer the cancel call, or do it
here, I think do it here makes more sense so we don't duplicate states into
workers.

[xingliu, 2017/03/21 23:27:59]

I'm also fine with let worker defer the cancel call, so Job just need to call
worker->Cancel, and worker can query states from Job through Delegate interface.
wdyt?

[qinmin, 2017/03/21 23:48:33]

Yes, it should be worker's responsibility to handle it

[xingliu, 2017/03/22 01:25:59]

Make sense, done.

+    return;
+  }
+
+  // Pause the stream if user paused, still push the stream reader to the sink.
+  if (DownloadJob::is_paused()) {
+    VLOG(kVerboseLevel) << "Byte stream arrived after user pause the request.";
+    worker->Pause();

[qinmin, 2017/03/21 22:53:55]

same here.

[xingliu, 2017/03/21 23:19:14]

We still need to add the stream to DownloadFileImpl on the pause case, so it can
write data. So can't bait out here, but let the logic fall into AddByteStream.

[qinmin, 2017/03/21 23:48:33]

Yes. what i mean is that you don't need to call Pause(). You still need to call
AddByteStream though

[xingliu, 2017/03/22 01:25:59]

Done. Moved to worker.

+  }
+
+  bool success = DownloadJob::AddByteStream(std::move(stream_reader),
+                                            worker->offset(), worker->length());
+
+  // Destroy the request if the sink is gone.
+  if (!success) {
+    VLOG(kVerboseLevel)
+        << "Byte stream arrived after download file is released.";
+    worker->Cancel();
+  }
 }
 
 void ParallelDownloadJob::OnServerResponseError(
     DownloadWorker* worker,
     DownloadInterruptReason reason) {
   // TODO(xingliu): Consider to let the original request to cover the full
   // content if the sub-requests get invalid response. Consider retry on certain
   // error.
   DownloadJob::Interrupt(reason);
 }
@@ skipping 70 matching lines @@
   // download request.
   download_params->set_referrer(Referrer(download_item_->GetReferrerUrl(),
                                          blink::WebReferrerPolicyAlways));
   // Send the request.
   worker->SendRequest(std::move(download_params));
   DCHECK(workers_.find(offset) == workers_.end());
   workers_[offset] = std::move(worker);
 }
 
 }  // namespace content