Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767593003:
Handle early pause, cancel for parallel requests. (Closed)
DescriptionHandle early pause, cancel for parallel requests.
Currently we have an issue that Pause, Cancel can be called before
building the parallel requests or before adding the byte stream reader
to the file sink.
In this case, we should still call request handle's pause or cancel to
block the pipe or destroy the pipe.
BUG=702683, 644352
Review-Url: https://codereview.chromium.org/2767593003
Cr-Commit-Position: refs/heads/master@{#459873}
Committed: https://chromium.googlesource.com/chromium/src/+/cca0315b59850a2d21409f77e6c9c970cb8a47e2
Patch Set 1 #Patch Set 2 : Rebase. #
Total comments: 9
Patch Set 3 : Work on feedback. #
Total comments: 5
Patch Set 4 : Work on feedback, let workers remember some states. #Patch Set 5 : Revert the change to let workers know more states. #Patch Set 6 : Remove a debugging code. #Patch Set 7 : Rebase with master. #Patch Set 8 : Removed empty line caused by auto merge. #Patch Set 9 : Work on feedback, make DownloadWorker cache some states. #
Total comments: 2
Patch Set 10 : Move is_canceled_ to subclass. #
Messages
Total messages: 58 (34 generated)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xingliu@chromium.org changed reviewers: + asanka@chromium.org, dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:96: worker->Cancel(); 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? https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:103: worker->Pause(); same here.
Thanks for the review, some discussion. https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:96: worker->Cancel(); On 2017/03/21 22:53:55, qinmin wrote: > 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? 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. https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:103: worker->Pause(); On 2017/03/21 22:53:55, qinmin wrote: > same here. 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.
Some discussion, I think it also makes sense to let the worker defer the cancel call. https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:96: worker->Cancel(); On 2017/03/21 22:53:55, qinmin wrote: > 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? 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?
https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:96: worker->Cancel(); On 2017/03/21 23:27:59, xingliu wrote: > On 2017/03/21 22:53:55, qinmin wrote: > > 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? > > 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? Yes, it should be worker's responsibility to handle it https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:103: worker->Pause(); On 2017/03/21 23:19:14, xingliu wrote: > On 2017/03/21 22:53:55, qinmin wrote: > > same here. > > 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. Yes. what i mean is that you don't need to call Pause(). You still need to call AddByteStream though
Thanks for the feedback, PTAL. Also clean up the unit tests. https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:96: worker->Cancel(); On 2017/03/21 23:48:33, qinmin wrote: > On 2017/03/21 23:27:59, xingliu wrote: > > On 2017/03/21 22:53:55, qinmin wrote: > > > 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? > > > > 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? > > Yes, it should be worker's responsibility to handle it Make sense, done. https://codereview.chromium.org/2767593003/diff/20001/content/browser/downloa... content/browser/download/parallel_download_job.cc:103: worker->Pause(); On 2017/03/21 23:48:33, qinmin wrote: > On 2017/03/21 23:19:14, xingliu wrote: > > On 2017/03/21 22:53:55, qinmin wrote: > > > same here. > > > > 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. > > Yes. what i mean is that you don't need to call Pause(). You still need to call > AddByteStream though Done. Moved to worker.
https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... File content/browser/download/parallel_download_job.h (right): https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... content/browser/download/parallel_download_job.h:58: bool IsPaused() const override; nit: worker can remember whether pause() is called or not, no need to ask the delegate https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... File content/browser/download/parallel_download_job_unittest.cc (right): https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... content/browser/download/parallel_download_job_unittest.cc:92: std::vector<DownloadWorkerForTest*> mock_workers_; looks like you only need to check the size of this and an iterator to loop through them, why not just use workers_ ? instead of introducing a new vector?
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the feedback. PTAL. Add a cancel check before building paralleld requests. Clean up the unit tests. https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... File content/browser/download/parallel_download_job.h (right): https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... content/browser/download/parallel_download_job.h:58: bool IsPaused() const override; On 2017/03/22 05:50:29, qinmin wrote: > nit: worker can remember whether pause() is called or not, no need to ask the > delegate We may pause/cancel before the worker is constructed. Alternatively we need to pass the current state to worker's constructor, which looks more weird. I think duplicate states can easily introduce bugs. This is caught in the unit test, when I move the is_pause into the worker. https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... File content/browser/download/parallel_download_job_unittest.cc (right): https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... content/browser/download/parallel_download_job_unittest.cc:92: std::vector<DownloadWorkerForTest*> mock_workers_; On 2017/03/22 05:50:29, qinmin wrote: > looks like you only need to check the size of this and an iterator to loop > through them, why not just use workers_ ? instead of introducing a new vector? Done, Good catch, this is no longer needed.
lgtm
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by xingliu@chromium.org
The CQ bit was checked by xingliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for content/browser/download/download_job.cc:
While running git apply --index -p1;
error: patch failed: content/browser/download/download_job.cc:52
error: content/browser/download/download_job.cc: patch does not apply
Patch: content/browser/download/download_job.cc
Index: content/browser/download/download_job.cc
diff --git a/content/browser/download/download_job.cc
b/content/browser/download/download_job.cc
index
1ed38b82fa8d83d02b5910ad2399e4b8780b35e6..5bd7aa086fac020802df5ab00c1a620b76cdfef7
100644
--- a/content/browser/download/download_job.cc
+++ b/content/browser/download/download_job.cc
@@ -12,10 +12,14 @@
namespace content {
DownloadJob::DownloadJob(DownloadItemImpl* download_item)
- : download_item_(download_item), is_paused_(false) {}
+ : download_item_(download_item), is_paused_(false), is_canceled_(false) {}
DownloadJob::~DownloadJob() = default;
+void DownloadJob::Cancel(bool user_cancel) {
+ is_canceled_ = true;
+}
+
void DownloadJob::Pause() {
is_paused_ = true;
}
@@ -33,17 +37,13 @@ void DownloadJob::Interrupt(DownloadInterruptReason reason)
{
download_item_->UpdateObservers();
}
-void DownloadJob::AddByteStream(std::unique_ptr<ByteStreamReader>
stream_reader,
+bool DownloadJob::AddByteStream(std::unique_ptr<ByteStreamReader>
stream_reader,
int64_t offset,
int64_t length) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
DownloadFile* download_file = download_item_->download_file_.get();
- if (!download_file) {
- // TODO(xingliu): Handle early and late incoming streams, see crbug/702683.
- VLOG(1) << "Download file is released when byte stream arrived. Discard "
- "the byte stream.";
- return;
- }
+ if (!download_file)
+ return false;
// download_file_ is owned by download_item_ on the UI thread and is always
// deleted on the FILE thread after download_file_ is nulled out.
@@ -52,6 +52,7 @@ void
DownloadJob::AddByteStream(std::unique_ptr<ByteStreamReader> stream_reader,
BrowserThread::FILE, FROM_HERE,
base::Bind(&DownloadFile::AddByteStream, base::Unretained(download_file),
base::Passed(&stream_reader), offset, length));
+ return true;
}
} // namespace content
https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... File content/browser/download/parallel_download_job.h (right): https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... content/browser/download/parallel_download_job.h:58: bool IsPaused() const override; On 2017/03/22 23:54:06, xingliu wrote: > On 2017/03/22 05:50:29, qinmin wrote: > > nit: worker can remember whether pause() is called or not, no need to ask the > > delegate > > We may pause/cancel before the worker is constructed. > > Alternatively we need to pass the current state to worker's constructor, which > looks more weird. I think duplicate states can easily introduce bugs. > > This is caught in the unit test, when I move the is_pause into the worker. wait, if we pause/cancel before worker is constructed, why do we create the workers.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/24 19:52:58, qinmin wrote: > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > File content/browser/download/parallel_download_job.h (right): > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > content/browser/download/parallel_download_job.h:58: bool IsPaused() const > override; > On 2017/03/22 23:54:06, xingliu wrote: > > On 2017/03/22 05:50:29, qinmin wrote: > > > nit: worker can remember whether pause() is called or not, no need to ask > the > > > delegate > > > > We may pause/cancel before the worker is constructed. > > > > Alternatively we need to pass the current state to worker's constructor, which > > looks more weird. I think duplicate states can easily introduce bugs. > > > > This is caught in the unit test, when I move the is_pause into the worker. > > wait, if we pause/cancel before worker is constructed, why do we create the > workers. Currently the worker is created when the request is sent. If we create the worker after the response is handled, I'm fine with that, but need a lot of code change here. Also it would be good to cache the data of the request we sent, so there will be an extra data structure to do that in ParallelDownloadJob.
On 2017/03/24 22:04:42, xingliu wrote: > On 2017/03/24 19:52:58, qinmin wrote: > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > File content/browser/download/parallel_download_job.h (right): > > > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > content/browser/download/parallel_download_job.h:58: bool IsPaused() const > > override; > > On 2017/03/22 23:54:06, xingliu wrote: > > > On 2017/03/22 05:50:29, qinmin wrote: > > > > nit: worker can remember whether pause() is called or not, no need to ask > > the > > > > delegate > > > > > > We may pause/cancel before the worker is constructed. > > > > > > Alternatively we need to pass the current state to worker's constructor, > which > > > looks more weird. I think duplicate states can easily introduce bugs. > > > > > > This is caught in the unit test, when I move the is_pause into the worker. > > > > wait, if we pause/cancel before worker is constructed, why do we create the > > workers. > > Currently the worker is created when the request is sent. > > If we create the worker after the response is handled, I'm fine with that, but > need a lot of code change here. Also it would be good to cache the data of the > request we sent, so there will be an extra data structure to do that in > ParallelDownloadJob. I think It's fine with both way, but how we define the worker, if it's only responsible for the http response part, then probably create after byte stream is better, and we won't have the async problem here, If it also carries some request data, then it should be created after the request is sent. wdyt?
On 2017/03/24 22:10:22, xingliu wrote: > On 2017/03/24 22:04:42, xingliu wrote: > > On 2017/03/24 19:52:58, qinmin wrote: > > > > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > > File content/browser/download/parallel_download_job.h (right): > > > > > > > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > > content/browser/download/parallel_download_job.h:58: bool IsPaused() const > > > override; > > > On 2017/03/22 23:54:06, xingliu wrote: > > > > On 2017/03/22 05:50:29, qinmin wrote: > > > > > nit: worker can remember whether pause() is called or not, no need to > ask > > > the > > > > > delegate > > > > > > > > We may pause/cancel before the worker is constructed. > > > > > > > > Alternatively we need to pass the current state to worker's constructor, > > which > > > > looks more weird. I think duplicate states can easily introduce bugs. > > > > > > > > This is caught in the unit test, when I move the is_pause into the worker. > > > > > > wait, if we pause/cancel before worker is constructed, why do we create the > > > workers. > > > > Currently the worker is created when the request is sent. > > > > If we create the worker after the response is handled, I'm fine with that, but > > need a lot of code change here. Also it would be good to cache the data of the > > request we sent, so there will be an extra data structure to do that in > > ParallelDownloadJob. > > I think It's fine with both way, but how we define the worker, if it's only > responsible for the http response part, then probably create after byte stream > is better, and we won't have the async problem here, > > If it also carries some request data, then it should be created after the > request is sent. > > wdyt? Worker creation has to depend on the response as we need the content-length header, right? I expect worker to only be created when Start() or Resume() is called, which we already have that logic in ParallelDownloadJob by using requests_sent_. Now while a worker is created, the download must be in a running state. So there will be no need to pass an is_pause variable into the ctor. The DownloadJob, can constantly call pause/cancel on the workers, and workers will remember the current state, rather than calling ParallelDownload::IsPaused().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/24 22:26:12, qinmin wrote: > On 2017/03/24 22:10:22, xingliu wrote: > > On 2017/03/24 22:04:42, xingliu wrote: > > > On 2017/03/24 19:52:58, qinmin wrote: > > > > > > > > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > > > File content/browser/download/parallel_download_job.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2767593003/diff/40001/content/browser/downloa... > > > > content/browser/download/parallel_download_job.h:58: bool IsPaused() const > > > > override; > > > > On 2017/03/22 23:54:06, xingliu wrote: > > > > > On 2017/03/22 05:50:29, qinmin wrote: > > > > > > nit: worker can remember whether pause() is called or not, no need to > > ask > > > > the > > > > > > delegate > > > > > > > > > > We may pause/cancel before the worker is constructed. > > > > > > > > > > Alternatively we need to pass the current state to worker's constructor, > > > which > > > > > looks more weird. I think duplicate states can easily introduce bugs. > > > > > > > > > > This is caught in the unit test, when I move the is_pause into the > worker. > > > > > > > > wait, if we pause/cancel before worker is constructed, why do we create > the > > > > workers. > > > > > > Currently the worker is created when the request is sent. > > > > > > If we create the worker after the response is handled, I'm fine with that, > but > > > need a lot of code change here. Also it would be good to cache the data of > the > > > request we sent, so there will be an extra data structure to do that in > > > ParallelDownloadJob. > > > > I think It's fine with both way, but how we define the worker, if it's only > > responsible for the http response part, then probably create after byte stream > > is better, and we won't have the async problem here, > > > > If it also carries some request data, then it should be created after the > > request is sent. > > > > wdyt? > > Worker creation has to depend on the response as we need the content-length > header, right? > I expect worker to only be created when Start() or Resume() is called, which we > already have that logic in ParallelDownloadJob by using requests_sent_. > Now while a worker is created, the download must be in a running state. So there > will be no need to pass an is_pause variable into the ctor. > The DownloadJob, can constantly call pause/cancel on the workers, and workers > will remember the current state, rather than calling > ParallelDownload::IsPaused(). I feel in this way, we actually can put all the logic into ParallelDownloadJob, if that sounds good, I'll start to make the change.
The main reason is UrlDownloader::Delegate need to be passed in when creating the request. So in this way, UrlDownloader::Delegate will be ParallelDownloadJob, and DownloadWorker now only hold a request_handle, which is trival, so we can just nuke it.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks. Work on the feedback, 1. Now DownloadWorker also has variable is_pause, is_cancel. 2. Add a DCHECK in BuildParallelRequests. 3. Removed an unit test, since it should never happen and guarded by the DCHECK we add.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... File content/browser/download/download_job.h (right): https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... content/browser/download/download_job.h:35: bool is_canceled() const { return is_canceled_; } nit: these functions and the private member variables are no longer needed in the base class
Thanks for the review. https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... File content/browser/download/download_job.h (right): https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... content/browser/download/download_job.h:35: bool is_canceled() const { return is_canceled_; } On 2017/03/26 05:01:00, qinmin wrote: > nit: these functions and the private member variables are no longer needed in > the base class We actually added an is_canceled check in BuildParallelRequest, to not send messages when the user canceled.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/27 17:29:33, xingliu wrote: > Thanks for the review. > > https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... > File content/browser/download/download_job.h (right): > > https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... > content/browser/download/download_job.h:35: bool is_canceled() const { return > is_canceled_; } > On 2017/03/26 05:01:00, qinmin wrote: > > nit: these functions and the private member variables are no longer needed in > > the base class > > We actually added an is_canceled check in BuildParallelRequest, to not send > messages when the user canceled. but that is in subclass, right? you don't need it in DownloadJob, only ParallelDownloadJob
On 2017/03/27 17:46:57, qinmin wrote: > On 2017/03/27 17:29:33, xingliu wrote: > > Thanks for the review. > > > > > https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... > > File content/browser/download/download_job.h (right): > > > > > https://codereview.chromium.org/2767593003/diff/160001/content/browser/downlo... > > content/browser/download/download_job.h:35: bool is_canceled() const { return > > is_canceled_; } > > On 2017/03/26 05:01:00, qinmin wrote: > > > nit: these functions and the private member variables are no longer needed > in > > > the base class > > > > We actually added an is_canceled check in BuildParallelRequest, to not send > > messages when the user canceled. > > but that is in subclass, right? you don't need it in DownloadJob, only > ParallelDownloadJob Done, move is_canceled_ to subclass.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/2767593003/#ps180001 (title: "Move is_canceled_ to subclass.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490647003970700,
"parent_rev": "c84e01bfb9b85b7703d00d9a27a57a792179f215", "commit_rev":
"cca0315b59850a2d21409f77e6c9c970cb8a47e2"}
Message was sent while issue was closed.
Description was changed from ========== Handle early pause, cancel for parallel requests. Currently we have an issue that Pause, Cancel can be called before building the parallel requests or before adding the byte stream reader to the file sink. In this case, we should still call request handle's pause or cancel to block the pipe or destroy the pipe. BUG=702683, 644352 ========== to ========== Handle early pause, cancel for parallel requests. Currently we have an issue that Pause, Cancel can be called before building the parallel requests or before adding the byte stream reader to the file sink. In this case, we should still call request handle's pause or cancel to block the pipe or destroy the pipe. BUG=702683, 644352 Review-Url: https://codereview.chromium.org/2767593003 Cr-Commit-Position: refs/heads/master@{#459873} Committed: https://chromium.googlesource.com/chromium/src/+/cca0315b59850a2d21409f77e6c9... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/cca0315b59850a2d21409f77e6c9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
