Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752603002:
Propagate server response error and interrupt the download. (Closed)
DescriptionPropagate server response error and interrupt the download.
This CL simply propagate anything to DownloadItemImpl, later we should
revisit it.
We need to wait for a couple of recent CL landed to start to test the
whole flow of parallel download.
BUG=644352
Review-Url: https://codereview.chromium.org/2752603002
Cr-Commit-Position: refs/heads/master@{#458519}
Committed: https://chromium.googlesource.com/chromium/src/+/391c49d74940a1f2c4fc4679d2426b9facef8939
Patch Set 1 #
Total comments: 11
Patch Set 2 : Work on feedbacks. #
Total comments: 6
Patch Set 3 : Work on feedbacks. #Patch Set 4 : Merge recent changes in the tree. #
Messages
Total messages: 30 (17 generated)
xingliu@chromium.org changed reviewers: + asanka@chromium.org, dtrainor@chromium.org, qinmin@chromium.org
Hi, PTAL, thanks.
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:56: void DownloadWorker::Pause() { Do we need to propagate any request state changes once we build the request_handle_? https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:81: if (create_info->save_info->offset != offset() || Just want to make sure, do we log this properly so we can track how often we're hitting this with parallel downloads? https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.h (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.h:61: DownloadWorker::Delegate* delegate_; DownloadWorker::Delegate* const delegate_? https://codereview.chromium.org/2752603002/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/pa... content/browser/download/parallel_download_job.cc:55: // TODO(xingliu): Consider to let the original request to cover the full Would this ever depend on how much content we've 'wasted'? Do we retry the job?
https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:81: if (create_info->save_info->offset != offset() || On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > Just want to make sure, do we log this properly so we can track how often we're > hitting this with parallel downloads? DownloadRequestCore::OnResponseStarted() already handles this case and interrupts with DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT. Why set result again?
Thanks for the review, PTAL again. https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:56: void DownloadWorker::Pause() { On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > Do we need to propagate any request state changes once we build the > request_handle_? Currently we don't, may added if we need more complex error handling. https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:81: if (create_info->save_info->offset != offset() || On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > Just want to make sure, do we log this properly so we can track how often we're > hitting this with parallel downloads? Make sense. Added log and TODO and remove the rewrite of result. The original idea to rewrite the result here is to make it not a hard failure, but it doesn't make sense since the etag has mismatched so the original request should retry the whole file. https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.h (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.h:61: DownloadWorker::Delegate* delegate_; On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > DownloadWorker::Delegate* const delegate_? Done. https://codereview.chromium.org/2752603002/diff/1/content/browser/download/pa... File content/browser/download/parallel_download_job.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/pa... content/browser/download/parallel_download_job.cc:55: // TODO(xingliu): Consider to let the original request to cover the full On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > Would this ever depend on how much content we've 'wasted'? Do we retry the job? Yes, we have the logic to limit the length of preceding streams in DownloadFileImpl when later streams starts, so we don't waste more data. When we report an error, DownloadItemImpl may restart the whole download. Added a TODO for retry, retry makes sense for certain error. Ideally if we refactor the DownloadItemImpl and Job owns File, then complex stream control can be done cleanly.
https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_job.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_job.cc:29: download_item_->InterruptAndDiscardPartialState(reason); This doesn't update the download_item_'s observers.
https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:81: if (create_info->save_info->offset != offset() || On 2017/03/14 19:52:10, asanka wrote: > On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > > Just want to make sure, do we log this properly so we can track how often > we're > > hitting this with parallel downloads? > > DownloadRequestCore::OnResponseStarted() already handles this case and > interrupts with DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT. Why set result > again? Sounds good! Just checking we have something in place to catch it. https://codereview.chromium.org/2752603002/diff/1/content/browser/download/do... content/browser/download/download_worker.cc:81: if (create_info->save_info->offset != offset() || On 2017/03/14 21:33:21, xingliu wrote: > On 2017/03/14 18:01:43, David Trainor-ping if over 24h wrote: > > Just want to make sure, do we log this properly so we can track how often > we're > > hitting this with parallel downloads? > > Make sense. Added log and TODO and remove the rewrite of result. > > The original idea to rewrite the result here is to make it not a hard failure, > but it doesn't make sense since the etag has mismatched so the original request > should retry the whole file. Yeah once the etag is mismatched I assume we have to toss out everything we already downloaded.
https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_worker.cc:84: create_info->save_info->length != length()) { This condition will trip for any half-open slice. But that's moot given the following. OnUrlDownloaderStarted() shouldn't be getting invoked with a mismatched slice. If the server didn't respond with the correct slice, that should be handled by DownloadRequestCore. Architecturally speaking, it would be brittle if we need to handle mismatched slices at this layer. The result we should expect at this point should be either an error, or a correct slice. Otherwise the all consumers downstream from DownloadRequestCore would need to handle a much larger set of possible outcomes than is practical. There's an additional complication where DownloadRequestCore doesn't have all the bits it needs to validate a server response. For example, the code there currently relies on heuristics to determine whether it's acceptable to receive the entire response when the request was for a slice. This is currently acceptable only for the primary resumption request. Unfortunately it's not always possible for DRC to tell the difference between the primary resumption request, and the last slice. Both of those are half-open and DRC accepts HTTP 200 responses for both. I'd suggest: * Explicitly indicate to DRC whether a full entity response is acceptable. Missing such an indication, DRC should strictly validate the response slice vs. the request rage. * Downstream from DRC, always assume that the response is valid. If the consumer explicitly indicated support for full entity responses, then they should check for that condition. Other than those, any deviation should have resulted in an error.
Thanks for the comments, CL polished. https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_job.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_job.cc:29: download_item_->InterruptAndDiscardPartialState(reason); On 2017/03/17 05:09:46, qinmin wrote: > This doesn't update the download_item_'s observers. Done, thanks for this suggestion. https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_worker.cc:84: create_info->save_info->length != length()) { On 2017/03/17 15:54:22, asanka wrote: > This condition will trip for any half-open slice. But that's moot given the > following. > > OnUrlDownloaderStarted() shouldn't be getting invoked with a mismatched slice. > If the server didn't respond with the correct slice, that should be handled by > DownloadRequestCore. > > Architecturally speaking, it would be brittle if we need to handle mismatched > slices at this layer. The result we should expect at this point should be either > an error, or a correct slice. Otherwise the all consumers downstream from > DownloadRequestCore would need to handle a much larger set of possible outcomes > than is practical. > > There's an additional complication where DownloadRequestCore doesn't have all > the bits it needs to validate a server response. For example, the code there > currently relies on heuristics to determine whether it's acceptable to receive > the entire response when the request was for a slice. This is currently > acceptable only for the primary resumption request. Unfortunately it's not > always possible for DRC to tell the difference between the primary resumption > request, and the last slice. Both of those are half-open and DRC accepts HTTP > 200 responses for both. > > > I'd suggest: > > * Explicitly indicate to DRC whether a full entity response is acceptable. > Missing such an indication, DRC should strictly validate the response slice vs. > the request rage. > > * Downstream from DRC, always assume that the response is valid. If the consumer > explicitly indicated support for full entity responses, then they should check > for that condition. Other than those, any deviation should have resulted in an > error. Good point, It will be cleaner to let DownloadRequestCore handle this. added a TODO here. I'll do this in a separate CL. We also need to pull the http version from there.
lgtm https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_worker.cc:84: create_info->save_info->length != length()) { On 2017/03/17 21:14:36, xingliu wrote: > On 2017/03/17 15:54:22, asanka wrote: > > This condition will trip for any half-open slice. But that's moot given the > > following. > > > > OnUrlDownloaderStarted() shouldn't be getting invoked with a mismatched slice. > > If the server didn't respond with the correct slice, that should be handled by > > DownloadRequestCore. > > > > Architecturally speaking, it would be brittle if we need to handle mismatched > > slices at this layer. The result we should expect at this point should be > either > > an error, or a correct slice. Otherwise the all consumers downstream from > > DownloadRequestCore would need to handle a much larger set of possible > outcomes > > than is practical. > > > > There's an additional complication where DownloadRequestCore doesn't have all > > the bits it needs to validate a server response. For example, the code there > > currently relies on heuristics to determine whether it's acceptable to receive > > the entire response when the request was for a slice. This is currently > > acceptable only for the primary resumption request. Unfortunately it's not > > always possible for DRC to tell the difference between the primary resumption > > request, and the last slice. Both of those are half-open and DRC accepts HTTP > > 200 responses for both. > > > > > > I'd suggest: > > > > * Explicitly indicate to DRC whether a full entity response is acceptable. > > Missing such an indication, DRC should strictly validate the response slice > vs. > > the request rage. > > > > * Downstream from DRC, always assume that the response is valid. If the > consumer > > explicitly indicated support for full entity responses, then they should check > > for that condition. Other than those, any deviation should have resulted in an > > error. > > Good point, It will be cleaner to let DownloadRequestCore handle this. > added a TODO here. > > I'll do this in a separate CL. We also need to pull the http version from there. Don't forget the part about explicitly indicating whether receiving the entire entity body is acceptable.
lgtm
Thanks. https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... File content/browser/download/download_worker.cc (right): https://codereview.chromium.org/2752603002/diff/20001/content/browser/downloa... content/browser/download/download_worker.cc:84: create_info->save_info->length != length()) { On 2017/03/20 20:50:20, asanka wrote: > On 2017/03/17 21:14:36, xingliu wrote: > > On 2017/03/17 15:54:22, asanka wrote: > > > This condition will trip for any half-open slice. But that's moot given the > > > following. > > > > > > OnUrlDownloaderStarted() shouldn't be getting invoked with a mismatched > slice. > > > If the server didn't respond with the correct slice, that should be handled > by > > > DownloadRequestCore. > > > > > > Architecturally speaking, it would be brittle if we need to handle > mismatched > > > slices at this layer. The result we should expect at this point should be > > either > > > an error, or a correct slice. Otherwise the all consumers downstream from > > > DownloadRequestCore would need to handle a much larger set of possible > > outcomes > > > than is practical. > > > > > > There's an additional complication where DownloadRequestCore doesn't have > all > > > the bits it needs to validate a server response. For example, the code there > > > currently relies on heuristics to determine whether it's acceptable to > receive > > > the entire response when the request was for a slice. This is currently > > > acceptable only for the primary resumption request. Unfortunately it's not > > > always possible for DRC to tell the difference between the primary > resumption > > > request, and the last slice. Both of those are half-open and DRC accepts > HTTP > > > 200 responses for both. > > > > > > > > > I'd suggest: > > > > > > * Explicitly indicate to DRC whether a full entity response is acceptable. > > > Missing such an indication, DRC should strictly validate the response slice > > vs. > > > the request rage. > > > > > > * Downstream from DRC, always assume that the response is valid. If the > > consumer > > > explicitly indicated support for full entity responses, then they should > check > > > for that condition. Other than those, any deviation should have resulted in > an > > > error. > > > > Good point, It will be cleaner to let DownloadRequestCore handle this. > > added a TODO here. > > > > I'll do this in a separate CL. We also need to pull the http version from > there. > > Don't forget the part about explicitly indicating whether receiving the entire > entity body is acceptable. Yeah, that's on my list.
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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...)
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 asanka@chromium.org, qinmin@chromium.org Link to the patchset: https://codereview.chromium.org/2752603002/#ps60001 (title: "Merge recent changes in the tree.")
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": 60001, "attempt_start_ts": 1490125272718880,
"parent_rev": "c8529921548accf5f93751f6f997bebe7e0683bd", "commit_rev":
"391c49d74940a1f2c4fc4679d2426b9facef8939"}
Message was sent while issue was closed.
Description was changed from ========== Propagate server response error and interrupt the download. This CL simply propagate anything to DownloadItemImpl, later we should revisit it. We need to wait for a couple of recent CL landed to start to test the whole flow of parallel download. BUG=644352 ========== to ========== Propagate server response error and interrupt the download. This CL simply propagate anything to DownloadItemImpl, later we should revisit it. We need to wait for a couple of recent CL landed to start to test the whole flow of parallel download. BUG=644352 Review-Url: https://codereview.chromium.org/2752603002 Cr-Commit-Position: refs/heads/master@{#458519} Committed: https://chromium.googlesource.com/chromium/src/+/391c49d74940a1f2c4fc4679d242... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/391c49d74940a1f2c4fc4679d242... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
